ICB-DCM / pyPESTO

python Parameter EStimation TOolbox
https://pypesto.readthedocs.io
BSD 3-Clause "New" or "Revised" License
207 stars 45 forks source link

Fix Optimization Summary failing after loading from hdf5 #1367

Closed PaulJonasJost closed 1 month ago

PaulJonasJost commented 3 months ago

if free_indices were interpreted as floats, we could not use them to subset the array. Happened to me after loading results from hdf5

codecov-commenter commented 3 months ago

Codecov Report

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

Project coverage is 84.55%. Comparing base (cacd525) to head (d578fa5). Report is 7 commits behind head on develop.

Files Patch % Lines
pypesto/result/optimize.py 66.66% 1 Missing :warning:

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

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #1367 +/- ## ======================================== Coverage 84.54% 84.55% ======================================== Files 157 157 Lines 12949 12950 +1 ======================================== + Hits 10948 10950 +2 + Misses 2001 2000 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dweindl commented 3 months ago

if free_indices were interpreted as floats, we could not use them to subset the array. Happened to me after loading results from hdf5

Thanks. However, I'd prefer fixing the root of the issue, rather than the symptom. I.e., ensuring free_indices is always int.

PaulJonasJost commented 3 months ago

if free_indices were interpreted as floats, we could not use them to subset the array. Happened to me after loading results from hdf5

Thanks. However, I'd prefer fixing the root of the issue, rather than the symptom. I.e., ensuring free_indices is always int.

So the save function should have taken care of this. Also Problem defines their free indices as int, therefore I am not sure where it went wrong except for the integration of free indices to the Optimizer result guaranteed it here, but will have one more look whether anything else might be missing. Will still leave it in the summary as well as a second safety measure.

dweindl commented 3 months ago

not sure where it went wrong

I don't see either how this could become float, except for the case of empty x_free_indices, which would be fixed by adding dtype=int to https://github.com/ICB-DCM/pyPESTO/blob/872e2530e55c944ee55f0e38bdbdb479d0c7fba5/pypesto/result/optimize.py#L198.

PaulJonasJost commented 3 months ago

@dweindl could you re-review please :) added the dtype argument but still kept the change before as an additional safety measure.

PaulJonasJost commented 2 months ago

still kept the change before as an additional safety measure

I don't think it makes sense to have it there. Either we specify and ensure that problem.x_free_indices contains int and assume that everywhere, or we specify it to be of any type of number and treat it accordingly everywhere.

IMO, it does make sense to do this here, as we only now start to ensure it is int, but for backwards compatibility or previous results, it might be good to have this as an additional safety net?

PaulJonasJost commented 1 month ago

Closing this for now.