Skip to content

Conversation

@Kucharssim
Copy link
Member

fixes #245

I have not implemented it for NumpyTransform because I was not sure about implementation w.r.t. serialization. I tried to make sure that if we cannot keep track of the jacobians, we raise an error rather than returning an incorrect output.

@Kucharssim Kucharssim requested a review from LarsKue April 17, 2025 10:11
@codecov
Copy link

codecov bot commented Apr 17, 2025

Codecov Report

Attention: Patch coverage is 88.80597% with 15 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
bayesflow/approximators/continuous_approximator.py 0.00% 4 Missing ⚠️
bayesflow/adapters/transforms/filter_transform.py 85.00% 3 Missing ⚠️
bayesflow/adapters/transforms/map_transform.py 86.95% 3 Missing ⚠️
bayesflow/adapters/transforms/drop.py 50.00% 1 Missing ⚠️
...sflow/adapters/transforms/elementwise_transform.py 50.00% 1 Missing ⚠️
bayesflow/adapters/transforms/keep.py 50.00% 1 Missing ⚠️
bayesflow/adapters/transforms/numpy_transform.py 50.00% 1 Missing ⚠️
bayesflow/adapters/transforms/transform.py 50.00% 1 Missing ⚠️
Files with missing lines Coverage Δ
bayesflow/adapters/adapter.py 82.77% <100.00%> (+1.14%) ⬆️
bayesflow/adapters/transforms/concatenate.py 87.69% <100.00%> (+3.37%) ⬆️
bayesflow/adapters/transforms/constrain.py 83.49% <100.00%> (+16.82%) ⬆️
bayesflow/adapters/transforms/log.py 100.00% <100.00%> (ø)
bayesflow/adapters/transforms/rename.py 93.33% <100.00%> (+0.47%) ⬆️
bayesflow/adapters/transforms/scale.py 100.00% <100.00%> (ø)
bayesflow/adapters/transforms/sqrt.py 93.75% <100.00%> (+2.84%) ⬆️
bayesflow/adapters/transforms/standardize.py 97.67% <100.00%> (+0.37%) ⬆️
bayesflow/adapters/transforms/drop.py 88.23% <50.00%> (-5.10%) ⬇️
...sflow/adapters/transforms/elementwise_transform.py 78.94% <50.00%> (-3.41%) ⬇️
... and 6 more

@LarsKue
Copy link
Contributor

LarsKue commented Apr 17, 2025

Thanks for the PR. Could you add tests for this behavior @Kucharssim? See e.g., tests/test_networks/test_inference_networks.py::test_density_numerically for a related test.

@Kucharssim
Copy link
Member Author

Yes, on it!

@stefanradev93 stefanradev93 deleted the branch bayesflow-org:dev April 22, 2025 14:37
@LarsKue
Copy link
Contributor

LarsKue commented Apr 22, 2025

This was accidentally closed. We will investigate how to restore the branch and reopen PRs.

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 implementing this! I noted some minor concerns, mostly regarding clarity.

@Kucharssim Kucharssim requested a review from LarsKue April 24, 2025 17:28
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 we are mostly ready to merge this as-is. Can you address the open conversation above and the comments from Copilot? Thanks!

@LarsKue LarsKue requested a review from Copilot April 25, 2025 17:48
Copy link
Contributor

Copilot AI left a 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 enhances the adapter functionality by adding support for tracking the log determinant of the Jacobians across transforms. Key changes include:

  • Introducing log_det_jac methods for several transforms (e.g. standardize, sqrt, scale, constrain, etc.).
  • Modifying the adapter’s forward and inverse methods to optionally return the log determinant of the Jacobian.
  • Adding comprehensive tests to verify the correct computation and exception handling for the Jacobian tracking.

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/test_adapters/test_adapters.py Updated tests to cover additional keys and jacobian tracking tests.
tests/test_adapters/conftest.py Introduced new fixtures that test the log_det_jac functionality.
bayesflow/approximators/continuous_approximator.py Modified forward method to incorporate and apply the change‐of‐variables formula.
bayesflow/adapters/transforms/* Added or updated log_det_jac methods across multiple transform files.
bayesflow/adapters/adapter.py Updated forward and inverse methods to support the log_det_jac flag.

@Kucharssim
Copy link
Member Author

Ok, I think I addressed everything now. If you are happy with this, feel free to merge

@LarsKue LarsKue merged commit a322ff1 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