Skip to content

Conversation

@rusty-electron
Copy link
Contributor

I will be adding more commits to this PR as per instructions from @LarsKue.

Updates to test fixtures:

  • tests/conftest.py: Modified the summary_network fixture to accept the summary_dim parameter.
  • tests/test_networks/conftest.py: Added new fixtures for summary_dim, key_dim, and various network instances (lst_net, set_transformer, set_transformer_key_dim_variation, deep_set).

New test cases:

  • tests/test_networks/test_summary_networks.py: Introduced new test cases test_compute_metrics and test_set_transformer_with_key_dim to verify the functionality of the new attributes and ensure the correctness of the network outputs.

@LarsKue
Copy link
Contributor

LarsKue commented Oct 11, 2024

Thank you for the PR. Please make sure that the linter passes before merging.

@LarsKue LarsKue added unit tests A new set of tests needs to be added. good first issue Good for first-time contributors labels Oct 11, 2024
@paul-buerkner
Copy link
Contributor

In an attempt to merge this PR, I noticed that there are some unknown arguments used in set_transformer, namely key_dim and num_attention_blocks.

@LarsKue I think you will need to check and fix this before you can merge it as I don't understand the context in which these changes were made.

@LarsKue
Copy link
Contributor

LarsKue commented Nov 25, 2024

@paul-buerkner please never merge PRs that do not at the very least pass all tests, unless you have very good reason to. I will look into this as soon as I have a time slot available.

@paul-buerkner
Copy link
Contributor

paul-buerkner commented Nov 25, 2024

I did not intend to merge this PR without consultation (even if all tests would have passed, which they are not because of the issue I brought up). I just wanted to move it forward. My earlier message was unclear in this regard. Sorry for that.

@paul-buerkner
Copy link
Contributor

In an attempt to move this PR forward, I realized that it's probably safer to make a new PR extending tests for summary networks, rather to rely on this one. It currently has too many loose ends that I am not quite able to fit together.

@paul-buerkner
Copy link
Contributor

I think I figured out what were the issues and was able to get all the tests running again. Changes to the code base outside of the tests are minimal. They only concern the consistent storing of summary_dim in all networks intended for summarizing. Previously this was only done for some summary networks but not for others which didn't make sense.

I will merge as soon as all checks are passing.

@codecov-commenter
Copy link

codecov-commenter commented Feb 27, 2025

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

Codecov Report

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

Files with missing lines Patch % Lines
...esflow/networks/transformers/fusion_transformer.py 0.00% 1 Missing ⚠️
...w/networks/transformers/time_series_transformer.py 0.00% 1 Missing ⚠️

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

Files with missing lines Coverage Δ
bayesflow/networks/transformers/set_transformer.py 93.75% <100.00%> (+0.20%) ⬆️
...esflow/networks/transformers/fusion_transformer.py 34.37% <0.00%> (-1.11%) ⬇️
...w/networks/transformers/time_series_transformer.py 25.58% <0.00%> (-0.61%) ⬇️

... and 1 file with indirect coverage changes

@paul-buerkner paul-buerkner merged commit fea5f89 into bayesflow-org:dev Feb 27, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

good first issue Good for first-time contributors unit tests A new set of tests needs to be added.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants