Skip to content

Conversation

@vpratz
Copy link
Collaborator

@vpratz vpratz commented Apr 10, 2025

Closes #362.

This PR reintroduces the features that were present in LambdaTransform, but only allowing registered functions. While being stricter, this allows for closer scaffolding and raising errors early on, so that users cannot provide functions that will not be (de)serializable later on.

As there are a few failure modes, the focus is on providing detailed error messages to enable users to solve problems without external help.

Open questions:

  • When writing the code, I chose the naming to make it very clear that you cannot pass any functions, but only serializable ones. In my impression, I overdid it a bit, and the point should be sufficiently clear, even when we rename serializable_forward/inverse_fn to forward/inverse again. What do you think?
  • I opted for a method in the Adapter, as it helps in easily providing the user with the FilterTransform functionality. Is this ok with you, @LarsKue
  • As always, naming suggestions are welcome.

This commit reintroduces the features that were present in
`LambdaTransform`, but only allowing registered functions. While being
stricter, that allows for closer scaffolding and raising errors early
on, so that users cannot provide functions that will not be
(de)serializable later on.

As there are a few failure modes, the focus is on providing detailed
error messages to enable users to solve problems without external help.
@codecov
Copy link

codecov bot commented Apr 10, 2025

Codecov Report

Attention: Patch coverage is 90.41096% with 7 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...apters/transforms/serializable_custom_transform.py 89.55% 7 Missing ⚠️
Files with missing lines Coverage Δ
bayesflow/adapters/adapter.py 80.87% <100.00%> (+0.42%) ⬆️
bayesflow/adapters/transforms/__init__.py 100.00% <100.00%> (ø)
...apters/transforms/serializable_custom_transform.py 89.55% <89.55%> (ø)

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.

Very awesome changes, thank you for covering this so thoroughly!

I do see the need for some minor changes, particularly in the tests. See individual comments.

@vpratz
Copy link
Collaborator Author

vpratz commented Apr 11, 2025

Thanks a lot for the review, I have addressed your comments. The tests are currently failing because I pulled in the changes from dev. If you have no objections, I will merge when the tests on dev are running again.

@vpratz vpratz requested a review from LarsKue April 11, 2025 16:29
@LarsKue
Copy link
Contributor

LarsKue commented Apr 11, 2025

@vpratz Yes, dev is now fixed. Can you please merge dev into this again and if tests pass, merge?

@vpratz vpratz merged commit 33920a2 into bayesflow-org:dev Apr 12, 2025
15 checks passed
@vpratz vpratz deleted the feat-serializable-custom-transform branch April 12, 2025 08:07
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.

2 participants