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 636 (2) #638

Merged
merged 7 commits into from
Apr 21, 2021
Merged

Fix 636 (2) #638

merged 7 commits into from
Apr 21, 2021

Conversation

PaulJonasJost
Copy link
Collaborator

adds as a fix to #636

@PaulJonasJost PaulJonasJost changed the title added exceptions if no result exists when reading in for profiling/op… Fix 636 (2) Apr 19, 2021
Copy link
Member

@dilpath dilpath left a comment

Choose a reason for hiding this comment

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

👍

pypesto/store/read_from_hdf5.py Outdated Show resolved Hide resolved
Comment on lines 300 to 301
logger.warning('Loading the problem failed. It is'
'highly likely that no problem exists.')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
logger.warning('Loading the problem failed. It is'
'highly likely that no problem exists.')
logger.warning('Loading the problem failed. It is'
'highly likely that no problem exists '
f'within {filename}.')

To match the other messages.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As Problem should always be written and read I corrected this to a regular warning that loading the problem failed. As of now I think most problems have amici objectives which have to be created again so that having the problem read in fail is in most cases no hindrance to the code.

Copy link
Member

Choose a reason for hiding this comment

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

Hm then it would be handled differently to sample/optimize/profile results. There, only KeyError is handled to treat a known issue, and other exceptions would be raised in the usual way. Here, all exceptions are caught and replaced with a logger.warning. So it may be better/more consistent to remove the try-except for the pypesto.Problem.

If there is a particular exception in mind here, better to use the specific constant, (e.g. KeyError instead of Exception).

Just a small point, fine to ignore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Think you are right, just removed it as there is no particular exception👍

pypesto/store/read_from_hdf5.py Outdated Show resolved Hide resolved
pypesto/store/read_from_hdf5.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Apr 21, 2021

Codecov Report

Merging #638 (11f8233) into develop (160c2a8) will increase coverage by 1.46%.
The diff coverage is 86.62%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #638      +/-   ##
===========================================
+ Coverage    88.16%   89.63%   +1.46%     
===========================================
  Files           79       93      +14     
  Lines         5257     5912     +655     
===========================================
+ Hits          4635     5299     +664     
+ Misses         622      613       -9     
Impacted Files Coverage Δ
pypesto/ensemble/covariance_analysis.py 18.36% <0.00%> (-0.39%) ⬇️
pypesto/objective/amici.py 85.56% <20.00%> (-4.27%) ⬇️
pypesto/predict/task.py 43.75% <43.75%> (ø)
pypesto/petab/__init__.py 63.63% <50.00%> (ø)
pypesto/engine/mpi_pool.py 66.66% <66.66%> (ø)
pypesto/objective/amici_util.py 84.21% <66.66%> (+8.31%) ⬆️
pypesto/ensemble/constants.py 91.30% <71.42%> (-8.70%) ⬇️
pypesto/visualize/misc.py 82.41% <72.22%> (-2.52%) ⬇️
pypesto/store/read_from_hdf5.py 91.20% <74.19%> (+33.30%) ⬆️
pypesto/visualize/waterfall.py 96.29% <83.33%> (ø)
... and 51 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c90479...11f8233. Read the comment docs.

@PaulJonasJost PaulJonasJost merged commit 91705c7 into develop Apr 21, 2021
@PaulJonasJost PaulJonasJost deleted the fix_636_2 branch April 21, 2021 10:26
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.

3 participants