Skip to content

Conversation

@paul-buerkner
Copy link
Contributor

This PR adds support for log and sqrt transforms as requested by @stefanradev93.

@LarsKue I noticed that we are using two different patterns to deal with keys in the adapter. One that uses keys directly in the internal adapter transforms (e.g., expand_dims) and another that loops over the keys inside the main Adapter (e.g., as_set). Is this international? If so, when shall we apply which pattern?

@codecov-commenter
Copy link

codecov-commenter commented Mar 10, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 96.15385% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
bayesflow/adapters/transforms/sqrt.py 83.33% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Files with missing lines Coverage Δ
bayesflow/adapters/adapter.py 76.39% <100.00%> (+1.90%) ⬆️
bayesflow/adapters/transforms/__init__.py 100.00% <100.00%> (ø)
bayesflow/adapters/transforms/expand_dims.py 100.00% <100.00%> (ø)
bayesflow/adapters/transforms/log.py 100.00% <100.00%> (ø)
bayesflow/adapters/transforms/sqrt.py 83.33% <83.33%> (ø)
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

Thanks for the PR! You can just use ElementwiseTransform without filtering for keys manually, this is already implemented by the FilterTransform. You can copy the pattern from AsSet. The ExpandDims transform is also incorrectly implemented.

@paul-buerkner
Copy link
Contributor Author

Thanks! That makes sense!

@paul-buerkner
Copy link
Contributor Author

Done. Can you check again @LarsKue ?

@LarsKue LarsKue self-requested a review March 11, 2025 09:05
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 changes. I think this looks good 👍

@paul-buerkner
Copy link
Contributor Author

Great! Merging now.

@paul-buerkner paul-buerkner merged commit 7537ee4 into dev Mar 11, 2025
14 checks passed
@paul-buerkner paul-buerkner deleted the log_sqrt_transforms branch March 11, 2025 10:37
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.

5 participants