Skip to content

Conversation

@vpratz
Copy link
Collaborator

@vpratz vpratz commented Apr 27, 2025

The functionality was replaced by the more powerful serialize and deserialize functions. Therefore I would suggest we deprecate the old functions.

It was still use in the point estimation setting, this change would break loading of previously saved models that use those classes. I just noticed that the serializable adapter is currently producing wrong packages (I will open and link an issue soon). I first thought that it was related to my changes, and already fixed it in a1b4d19. But the problem is also present on dev and main.
As the fix for this also breaks backwards compatibility, I think doing this deprecation now might be a good.

This PR is a continuation of #449 which also contains the fix, so the diff will simplify once #449 is merged (or we merge both jointly by merging this one).

vpratz added 4 commits April 27, 2025 13:27
* add functools.wraps call to allow_kwargs decorator, as before it was
  breaking the autodoc functionality
* move content to separate pages
* update section on serialization
The functools.wrap decorator adds a frame object to the call stack
@codecov
Copy link

codecov bot commented Apr 27, 2025

Codecov Report

Attention: Patch coverage is 86.95652% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
bayesflow/utils/serialization.py 75.00% 3 Missing ⚠️
Files with missing lines Coverage Δ
bayesflow/networks/point_inference_network.py 78.94% <100.00%> (-0.44%) ⬇️
bayesflow/scores/scoring_rule.py 93.61% <100.00%> (ø)
bayesflow/utils/__init__.py 100.00% <100.00%> (ø)
bayesflow/utils/decorators.py 92.30% <100.00%> (+1.39%) ⬆️
bayesflow/utils/serialization.py 90.69% <75.00%> (+0.13%) ⬆️

@vpratz
Copy link
Collaborator Author

vpratz commented Apr 27, 2025

I cannot get the PointInferenceNetwork to work with the new functions. @han-ol Do you have time to take a look at this?
Edit: Not making other changes to the serialization in the classes helped, I think this is ready to be reviewed.

- add deprecation warning, remove functionality
- replace all occurences with the corresponding new functions
@vpratz vpratz force-pushed the deprecate-old-serialization branch from 161593c to 06e3352 Compare April 27, 2025 20:34
@vpratz vpratz marked this pull request as ready for review April 27, 2025 20:47
@vpratz vpratz requested review from LarsKue and han-ol April 28, 2025 08:08
@han-ol
Copy link
Collaborator

han-ol commented Apr 29, 2025

Thanks for looking into this and glad that it worked out. With respect to serialization changes of PointInferenceNetwork I have nothing to complain.

I take it you also tried to address the awkwardness documented in https://github.com/bayesflow-org/bayesflow/blob/main/bayesflow/networks/point_inference_network.py#L80. Do you think we can make progress here before the upstream keras issue is resolved?

In any case, this PR is ready to merge after @LarsKue takes a look.

@vpratz
Copy link
Collaborator Author

vpratz commented Apr 29, 2025

Thanks for your comments. Regarding your question: Yes, in my first attempt I tried to streamline it a bit, but it failed with error messages I could not learn a lot from, unfortunately. They might well be related to the issue, but I did not explore it further. As the new functions do a very similar thing as the old ones, my guess would be that we cannot resolve this yet.

@LarsKue LarsKue changed the title Deprecate old serialization ((de)serialize_value_or_type) Deprecate old serialization ((de)serialize_value_or_type) and add developer docs Apr 29, 2025
Copy link
Contributor

@LarsKue LarsKue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR and for the added docs!

@LarsKue LarsKue merged commit 5595eab into bayesflow-org:dev Apr 29, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants