Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check if 'cnn_output_size' is zero in CNNembedding. #1281

Merged

Conversation

Ankush7890
Copy link
Contributor

When configuring the CNN-based embedding layer using sbi.neural_nets.embedding_nets.CNNEmbedding, setting the parameters for num_cnn_layers, kernel_size, and pool_kernel_size incorrectly may result in a vague error message, such as:

RuntimeError: Given input size: (6x4x4). Calculated output size: (6x0x0). Output size is too small.

Proposed Solution:
Since the cnn_output_size is recalculated after each layer, a CNN setup with excessive depth results in an output size of zero. This could be leveraged to alert the user about appropriate layer size configurations or to adjust other parameters accordingly.

Copy link
Contributor

@michaeldeistler michaeldeistler left a comment

Choose a reason for hiding this comment

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

Great! Thank you!!

@michaeldeistler
Copy link
Contributor

Unfortunately, ruff linting is not passing:
Screenshot 2024-09-20 at 07 11 22

Please run ruff format sbi and ruff check sbi --fix.

@Ankush7890
Copy link
Contributor Author

Thank you for letting me know. Pushed ruff linting related fixes 👍 .

@michaeldeistler
Copy link
Contributor

Great! Ruff is passing now, I will merge once all other tests are also through. Thanks again!

@michaeldeistler
Copy link
Contributor

michaeldeistler commented Sep 20, 2024

Unfortunately, one of the tests has failed. To reproduce:
pytest tests/embedding_net_test.py -k test_1d_and_2d_cnn_embedding_net

This gives

            cnn_output_size = get_new_cnn_output_size(cnn_output_size, conv_layer, pool)
            assert (
>               cnn_output_size[0] > 0 and cnn_output_size[1] > 0
            ), f"""CNN output size is zero at layer {ii + 1}. Either reduce
                 num_cnn_layers to {ii} or adjust the kernel_size
                 and pool_kernel_size accordingly."""
E           IndexError: tuple index out of range

Could you have a look?

@Ankush7890
Copy link
Contributor Author

Made the fix, and confirmed with the tests. This should be okay now 👍. Sorry, missed the variability with 1D convs before.

Copy link

codecov bot commented Sep 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.39%. Comparing base (299854e) to head (9b0018c).
Report is 9 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1281       +/-   ##
===========================================
- Coverage   89.24%   78.39%   -10.86%     
===========================================
  Files         119      119               
  Lines        8695     8849      +154     
===========================================
- Hits         7760     6937      -823     
- Misses        935     1912      +977     
Flag Coverage Δ
unittests 78.39% <100.00%> (-10.86%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
sbi/neural_nets/embedding_nets/cnn.py 100.00% <100.00%> (ø)

... and 35 files with indirect coverage changes

@Ankush7890
Copy link
Contributor Author

@michaeldeistler, Just curious, does this PR needs more modification? Or you think this addition would not be much useful?

Thanks.

@Ankush7890 Ankush7890 closed this Sep 27, 2024
@Ankush7890 Ankush7890 reopened this Sep 27, 2024
@michaeldeistler
Copy link
Contributor

Hi there, sorry I am currently travelling and therefore was a bit slow to have another look. I just ran tests, if they pass then I will merge. Thanks a lot for the contribution!

@Ankush7890
Copy link
Contributor Author

Ah, I see. No worries. It seems all the tests pass now. Thanks, you can merge it whenever it is possible for you.

Also, It's an amazing opensource library. Thanks for creating it.

@michaeldeistler michaeldeistler merged commit 138c5dc into sbi-dev:main Oct 2, 2024
12 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.

2 participants