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

delete_nan_inf can handle list input type #1243

Merged
merged 10 commits into from
Dec 20, 2023

Conversation

m-philipps
Copy link
Contributor

@m-philipps m-philipps commented Dec 8, 2023

@codecov-commenter
Copy link

codecov-commenter commented Dec 8, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (7237234) 83.87% compared to head (cf1bbcb) 83.87%.

Files Patch % Lines
pypesto/util.py 85.71% 1 Missing ⚠️

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

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1243   +/-   ##
========================================
  Coverage    83.87%   83.87%           
========================================
  Files          148      148           
  Lines        12067    12071    +4     
========================================
+ Hits         10121    10125    +4     
  Misses        1946     1946           

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

@m-philipps m-philipps marked this pull request as ready for review December 9, 2023 10:47
@m-philipps m-philipps requested review from stephanmg and a team as code owners December 9, 2023 10:47
@@ -375,7 +375,7 @@ def stats_lowlevel(
The plot axes.
"""
fvals = result.optimize_result.fval
values = [res[property_name] for res in result.optimize_result.list]
values = [[res[property_name]] for res in result.optimize_result.list]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is making that specifically a list within a list required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is making the use of delete_nan_inf consistent.
The values are given over to delete_nan_inf in place of the optimization result parameters. These are parameter vectors within a list, i.e. one list (or None) per start.

@m-philipps
Copy link
Contributor Author

Does anyone know where this condition is needed? It seems counter-intuitive because the function deletes nan values from the given list.

pyPESTO/pypesto/util.py

Lines 282 to 296 in c012c19

else:
# If none of the fvals are finite we try to construct an
# empty np.ndarray with the correct dimension (other functions rely
# on x.shape[1] to be of correct dimension)
x = np.array(x)
x = np.empty(
(
0,
x.shape[1]
if x.ndim == 2
else x[0].shape[0]
if x[0] is not None
else xdim,
)
)

@dweindl
Copy link
Member

dweindl commented Dec 19, 2023

Does anyone know where this condition is needed? It seems counter-intuitive because the function deletes nan values from the given list.

No idea where it's needed. Yes, it's deletes the nans, but it makes sure that the dimensions of x still match (an np.array with shape (0, 0) is different from (0, 2), although they are both empty).

Comment on lines +124 to +134

# parse fvals and parameters
fvals = np.array(fvals)
# remove nan or inf values
xs, fvals = delete_nan_inf(
fvals=fvals,
x=xs,
xdim=len(ub) if ub is not None else 1,
magnitude_bound=WATERFALL_MAX_VALUE,
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Moving the delete_nan_inf to here makes it so the plotting of reference points doesn't go through it. Is that an issue? I've never used reference points, but they also possibly might contain infinite/nan values, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, it's not a problem though, nans are just not shown in the line plot.

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.

Looks fine, branch needs update.

@m-philipps m-philipps merged commit b4b3ac4 into ICB-DCM:develop Dec 20, 2023
18 checks passed
@m-philipps m-philipps deleted the fix_1107 branch December 20, 2023 08:53
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.

Looks good! Thank you

This was referenced Jan 30, 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.

None yet

6 participants