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

fix: handle negative model predictions in visualizations #394

Merged
merged 6 commits into from
Apr 1, 2023

Conversation

rmnmllr
Copy link
Contributor

@rmnmllr rmnmllr commented Mar 23, 2023

related to issue #388.

Tried python -m pytest, did not pass all tests even before modifying anything. No additional errors though after the fix and tested with samples mantioned in issue #388.

@rmnmllr
Copy link
Contributor Author

rmnmllr commented Mar 24, 2023

@alexander-held I'm not sure why the python tests fail, pytest gives an error

>       assert "predicted yield is zero in 1 bin(s), excluded from ratio plot" in [
            rec.message for rec in caplog.records
        ]
E       AssertionError: assert 'predicted yield is zero in 1 bin(s), excluded from ratio plot' in ['predicted yield is zero in 1 bin(s),            excluded from ratio plot']

but this is not something I touched.

@alexander-held
Copy link
Member

Thanks a lot for preparing this! I am afraid it may take me until next week to more carefully look at this, sorry for the slow feedback here.

@rmnmllr
Copy link
Contributor Author

rmnmllr commented Mar 27, 2023

Thanks a lot for preparing this! I am afraid it may take me until next week to more carefully look at this, sorry for the slow feedback here.

No worries, I'm glad I can contribute in some way!
And no rush with this, I can run stuff already locally :)

Roman Mueller added 2 commits April 1, 2023 15:13
…ata plots a ValueError is raised if the total yields are negative.
@codecov
Copy link

codecov bot commented Apr 1, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (4645cb8) 100.00% compared to head (89b4170) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #394   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        23           
  Lines         2059      2065    +6     
  Branches       324       326    +2     
=========================================
+ Hits          2059      2065    +6     
Impacted Files Coverage Δ
src/cabinetry/visualize/plot_model.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@alexander-held
Copy link
Member

Hi @rmnmllr, I added tests to this to get this ready for inclusion in the next version of cabinetry that I intend to release with #390 also being completed. Thanks a lot for preparing this!

* catch negative total model prediction in visualize.plot_model.data_mc
* handle negative nominal template prediction in visualize.plot_model.templates

@alexander-held alexander-held changed the title fix: negative yerr in ratio plots fix: handle negative model predictions in visualizations Apr 1, 2023
@rmnmllr
Copy link
Contributor Author

rmnmllr commented Apr 3, 2023

Thank you @alexander-held for completing this!

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.

None yet

2 participants