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 error-bar bug for normalized consistency plots #222

Merged
merged 7 commits into from
May 31, 2023

Conversation

pabloitu
Copy link
Collaborator

@pabloitu pabloitu commented Apr 16, 2023

Type of change:

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes

@codecov-commenter
Copy link

codecov-commenter commented Apr 16, 2023

Codecov Report

Patch coverage: 60.43% and project coverage change: +0.27 🎉

Comparison is base (e50167f) 54.36% compared to head (29acbd4) 54.63%.

❗ Current head 29acbd4 differs from pull request most recent head c4353c9. Consider uploading reports for the commit c4353c9 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #222      +/-   ##
==========================================
+ Coverage   54.36%   54.63%   +0.27%     
==========================================
  Files          21       22       +1     
  Lines        3802     3904     +102     
  Branches      636      650      +14     
==========================================
+ Hits         2067     2133      +66     
- Misses       1603     1636      +33     
- Partials      132      135       +3     
Impacted Files Coverage Δ
csep/__init__.py 26.28% <5.55%> (-2.71%) ⬇️
csep/utils/readers.py 46.08% <28.57%> (-0.37%) ⬇️
csep/core/poisson_evaluations.py 43.24% <48.57%> (ø)
csep/core/catalogs.py 61.02% <66.66%> (+0.06%) ⬆️
csep/utils/geonet.py 81.57% <81.57%> (ø)

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

@fabiolsilva
Copy link
Contributor

Pablo,

I just noticed you deleted the MANIFEST.in file in your pull request. Was this intentional? Could you take a look?

Thanks!

@pabloitu
Copy link
Collaborator Author

pabloitu commented May 2, 2023

nice catch, sorry bout that. fixed

Copy link
Contributor

@fabiolsilva fabiolsilva 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 including a test! Only very very minor changes related to formatting and indentation. Please take a look at the comments for specific examples on what to look for. Once revised, these changes should be added to the main branch for the release.

csep/utils/plots.py Outdated Show resolved Hide resolved
pyplot.errorbar(bin_edges_plot, u3etas_median, yerr=[u3etas_emin, u3etas_emax], xerr=0.8 * dmw / 2, fmt=' ',
pyplot.errorbar(bin_edges_plot, u3etas_median,
yerr=[u3etas_emin, u3etas_emax], xerr=0.8 * dmw / 2,
fmt=' ',
label=sim_label, color='blue', alpha=0.7)
Copy link
Contributor

Choose a reason for hiding this comment

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

Move these up to the line above, so fmt is not by itself.

Comment on lines +2032 to +2037
plow97 = scipy.stats.poisson.ppf((1 - percentile / 100.) / 2.,
test_results[
i].test_distribution[1])
phigh97 = scipy.stats.poisson.ppf(
1 - (1 - percentile / 100.) / 2.,
test_results[i].test_distribution[1])
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, these two (and other similar lines throughout the file) should be indented the same way.

csep/utils/plots.py Outdated Show resolved Hide resolved
csep/core/poisson_evaluations.py Outdated Show resolved Hide resolved
@fabiolsilva fabiolsilva merged commit 97ba99e into SCECcode:master May 31, 2023
@fabiolsilva
Copy link
Contributor

Tests initially passed but recently failed due to setting up conda environment issue, merge approved and tests passed after switching to mamba for building the action environment (see PR #228).

pabloitu added a commit to pabloitu/pycsep that referenced this pull request May 31, 2023
* fix: matplotlib errorbar now doesnt throw exception in normalized consistency tests
tests: added unit test for comparative plots

* fix: removed numpy.warnings in favor of warnings module

* increased the number of comparison plot testss, and set to 2 the minimum of Mock models.

* forced errorbar limit arguments of consistency plot to be positive, which caused problems with precision rounding

* small detail: defaulted t-test plot axis labels to x: None,  y: Information Gain

* returned Manifest

* fixed some style issues
@pabloitu pabloitu deleted the hotfix_plots branch August 16, 2024 13:29
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.

numpy deprecated function
3 participants