-
Notifications
You must be signed in to change notification settings - Fork 63
Improve serialization and allow networks to be passed directly to most models #413
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
deprecate subnet_kwargs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 60 out of 60 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
bayesflow/experimental/resnet/resnet.py:33
- Reassigning the 'activation' parameter within the loop may cause unintended behavior in subsequent iterations. Consider using a separate variable (e.g., 'layer_activation') for each iteration to preserve the original configuration.
activation = keras.activations.get(activation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves the serialization approach for transforms and adapters by replacing the legacy Keras serialization calls with the new bayesflow.utils.serialization methods. In addition, it refactors some APIs (for example, renaming internal parameters) and updates configuration methods to use a unified serialization format.
- Updated all serialization decorators and get_config methods to use bayesflow’s new serialization utilities.
- Removed redundant from_config methods in many transform classes.
- Renamed internal parameters (e.g. _indices → indices) to improve clarity.
Reviewed Changes
Copilot reviewed 93 out of 93 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
bayesflow/adapters/transforms/sqrt.py | Updated serialization decorator and removed redundant from_config method |
bayesflow/adapters/transforms/shift.py | Switched to bayesflow serialization and updated get_config implementation |
bayesflow/adapters/transforms/scale.py | Switched to bayesflow serialization and updated get_config implementation |
bayesflow/adapters/transforms/rename.py | Switched to bayesflow serialization for get_config |
bayesflow/adapters/transforms/one_hot.py | Updated serialization usage in get_config |
bayesflow/adapters/transforms/numpy_transform.py | Updated get_config to serialize function names |
bayesflow/adapters/transforms/map_transform.py | Switched to bayesflow serialization for get_config |
bayesflow/adapters/transforms/log.py | Updated serialization usage in get_config |
bayesflow/adapters/transforms/keep.py | Updated serialization usage in get_config |
bayesflow/adapters/transforms/filter_transform.py | Updated get_config implementation with kwargs merge and updated serialization |
bayesflow/adapters/transforms/expand_dims.py | Updated serialization usage in get_config |
bayesflow/adapters/transforms/elementwise_transform.py | Changed from_config to use deserialize and init parameters |
bayesflow/adapters/transforms/drop.py | Updated serialization usage in get_config |
bayesflow/adapters/transforms/convert_dtype.py | Updated serialization usage and changed astype calls to use copy=False |
bayesflow/adapters/transforms/constrain.py | Updated serialization usage in get_config |
bayesflow/adapters/transforms/concatenate.py | Renamed internal parameter from _indices to indices and updated get_config accordingly |
bayesflow/adapters/transforms/broadcast.py | Updated serialization usage in get_config |
bayesflow/adapters/transforms/as_time_series.py | Updated serialization decorator |
bayesflow/adapters/transforms/as_set.py | Updated serialization decorator |
bayesflow/adapters/adapter.py | Updated serialization for adapter configuration |
Comments suppressed due to low confidence (3)
bayesflow/adapters/transforms/concatenate.py:35
- Renaming the parameter from '_indices' to 'indices' improves clarity, but please ensure that all references in this class are updated accordingly.
def __init__(self, keys: Sequence[str], *, into: str, axis: int = -1, indices: list | None = None):
bayesflow/adapters/transforms/filter_transform.py:80
- Merging 'self.kwargs' into the configuration dictionary may unintentionally override keys such as 'include' or 'predicate' if present; consider explicitly handling potential key collisions.
config = {"include": self.include, "transform_constructor": self.transform_constructor, "predicate": self.predicate, "exclude": self.exclude, "transform_map": self.transform_map, **self.kwargs}
bayesflow/adapters/transforms/convert_dtype.py:35
- Using 'copy=False' in astype may lead to in-place modifications; verify that this behavior is intentional and does not cause unintended side effects.
return data.astype(self.to_dtype, copy=False)
Tests are passing, I would say this is ready to merge! 🚀 |
This PR includes:
from_config
andget_config
methods for most models which allow the following patternnetwork.summary(expand_nested=True)
via subclassing fromkeras.Model
instead ofkeras.Layer
build
andcompute_output_shape
methodsNote that the
subnet_kwargs
pattern is now deprecated in networks that do not require it. We might aim to remove this feature in the future.Some drawbacks:
keras.Model
, our serialization testing became much more strict. This means that individual layers of each model must now also conform to consistent naming, i.e. the name cannot change e.g. fromdense_1
todense_2
. This usually means that models must name their layers statically. I have disabled the test for this for now, since implementing this in one go is a lot of work, but we should aim to re-enable this test in the future.from_config
andget_config
method now do, even when they only have python primitives as constructor arguments (due to the above issue).PointInferenceNetwork
and its constituents are currently exempt from improvements as they posed a significant hurdle in conversion. Perhaps @han-ol can port them to the new idiom, after some correspondence.Closes #343
Closes #391