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

Correlation plot with nans #1365

Merged
merged 8 commits into from
May 2, 2024
Merged

Conversation

PaulJonasJost
Copy link
Collaborator

Added a test. There was previously an error, when "all" start indices were selected, where no general checks were run.

…ectly. Also safety checks for startindices like "all" or "clustered"
…ty checks in general for start indices. Added a test for correlation matrix
@codecov-commenter
Copy link

codecov-commenter commented Apr 9, 2024

Codecov Report

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

Project coverage is 84.51%. Comparing base (3edf785) to head (dcb1a06).

Files Patch % Lines
pypesto/visualize/misc.py 77.77% 2 Missing ⚠️

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

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1365      +/-   ##
===========================================
+ Coverage    84.42%   84.51%   +0.08%     
===========================================
  Files          157      157              
  Lines        12928    12931       +3     
===========================================
+ Hits         10915    10928      +13     
+ Misses        2013     2003      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@stephanmg stephanmg left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@Doresic Doresic left a comment

Choose a reason for hiding this comment

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

The process_start_indices function is used by multiple visualization routines. And some of them also have specific ways of dealing with infinite values. I think those should then be updated as well accordingly if process_start_indices doesn't return start indices with infinite values anymore. At least removed, so that the start indices are not checked for infinite values.

We should also think about adding a warning or logging entry that some start indices were removed from plotting due to infinite values (if that's the case). That's, for instance, done in the parameters.py plotting routine.

Comment on lines +366 to +367
if np.isfinite(result.optimize_result[start_index].fval)
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check for np.isnan here as well?

Copy link
Contributor

@stephanmg stephanmg Apr 11, 2024

Choose a reason for hiding this comment

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

np.isfinite tests for Not a Number already, in case of NaN returns False.

@stephanmg
Copy link
Contributor

stephanmg commented Apr 11, 2024

We should also think about adding a warning or logging entry that some start indices were removed from plotting due to infinite values (if that's the case). That's, for instance, done in the parameters.py plotting routine.

Could that be as simple as a string message / status with: "Removed XY entries due to infinite or NaN value"?

Copy link
Contributor

@m-philipps m-philipps left a comment

Choose a reason for hiding this comment

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

Looks good to me, I would update the process_start_indices description, e.g.:

Create an array of indices if a number was provided, checks that the
indices do not exceed the max_index and removes starts with non-finite fval.

@PaulJonasJost PaulJonasJost merged commit b95f422 into develop May 2, 2024
18 checks passed
@PaulJonasJost PaulJonasJost deleted the correlation_plot_with_nans branch May 2, 2024 08:52
This was referenced May 27, 2024
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.

5 participants