-
Notifications
You must be signed in to change notification settings - Fork 124
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
Add function to plot Impf variability of calibration #791
Add function to plot Impf variability of calibration #791
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the method .plot_impf_variability()
to the OutputEvaluator
! I believe this nice and informative visualisation is very helpful when calibrating impact functions. My comments:
- code comments could be a bit more specific (see suggestions in
base.py
) - don't fully understand the reasoning to show the intermediate step of getting the
haz_vals
before plotting (seeclimada_util_calibrate.ipynb
) - several trailing whitespace Linter issues (VS can remove the automatically, https://linux.how2shout.com/remove-trailing-spaces-automatically-in-visual-code-studio/)
- complete the PR checklist (just now saw that the PR is only a draft...uuuups. I can give a final review once you're fully ready).
Thanks @leonie-villiger for the review, especially the automatic whitespace removal tip! :) @peanutfun I managed to get the histogram much faster by using the .data attribute. Do you want to have a look over it before I merge it? The failing unit test are not related to this specific pull request, and I didn't add any tests for the new plotting function for now (as with the other plotting function in the calibration module). Is that all fine? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timschmi95 Very helpful contribution, thanks a lot for that! My main concerns are how to deal with multiple impact functions within the impact function set and with the handling of plot parameters.
The failing tests are fine, we will fix this in the "main" PR.
@peanutfun Thanks for the review, let me know if you think the added changes are suitable. Currently, the function will only return the last axis object in case there are multiple impact functions, but I think this shouldn't be an issue? |
Co-authored-by: Lukas Riedel <[email protected]>
Co-authored-by: Lukas Riedel <[email protected]>
Changes proposed in this PR:
As discussed at the retreat. For me this function was quite useful to get a "feel" for the data. In some cases, it highlighted completely different impact functions with an almost equal cost-function value. Let me know if you think such a function is useful for users in general.
The two added plots and description are in the Section "Analyze the Calibration" of the tutorial Jupyter notebook.
PR Author Checklist
develop
)PR Reviewer Checklist