-
Notifications
You must be signed in to change notification settings - Fork 5
Added radio buttons to select histogram method for surface fields #1173 #1303
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
base: main
Are you sure you want to change the base?
Conversation
Fixes #1173 to allow users to select the method for computing histograms of surface fields. Possible methods are - frequency, normalised frequency, density. |
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.
The overall design looks very sensible and it should provide a welcome improvement in our histogram plots.
I've left a few comments throughout, but the two big things are adding tests for the histogram function, and considering whether we are right in making this user configurable outside of the recipes. See this comment.
fixed return from _get_histogram_method_surface Co-authored-by: James Frost <[email protected]>
Co-authored-by: James Frost <[email protected]>
Co-authored-by: James Frost <[email protected]>
Co-authored-by: James Frost <[email protected]>
removing the `duff` definition from the dictionary in `_plot_histogram` Co-authored-by: James Frost <[email protected]>
cleaned up the error reporting in `_plot_histogram` Co-authored-by: James Frost <[email protected]>
Sorry for being slow to review this. I'll go ahead and update the branch myself to fix the conflicting changes. |
I've also been a bit busy James. I've tried to implement your suggested edits, but I've yet to write a test for the new histogram plotting function. I can also follow up on your suggestion to move the histogram method to the Setup. |
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.
Its looking good with the changes you have done. I've just brought the branch up-to-date with the trunk, so once we have a few more tests we will be ready to merge.
Contribution checklist
Aim to have all relevant checks ticked off before merging. See the developer's guide for more detail.