-
Notifications
You must be signed in to change notification settings - Fork 7
Added radio buttons to select histogram method for surface fields #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.
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.
I'll rebase ahead of full review. |
34eec02
to
9a610ac
Compare
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.
Looks good and works well. A really valuable change to CSET.
One very minor comment worth thinking about (also partly not sure if it was something I changed in the re-base that has altered it).
I was wondering if it was worth putting a little bit of information in the help text to indicate that the method selection appears in General Setup Options
or seeing if there is a way it can be moved to appear in the location where the histograms are selected.
Also worth noting there should probably be a separate pull result to allow pressure levels and model level histograms to be selected in the same way as it currently does not trigger for those).
Originally the histogram method was selected where the histogram options appear. So yes, it might be worth popping this function back there. This enhancement was designed to allow someone to select a single method for producing histograms. It might be worth considering allowing multiple methods to be selected, similar to how "MEAN", "MIN" and "MAX" can be selected for producing aggregated plots. |
I'm tempted to suggest that we should add this as an argument to the histogram operator, and then use a recipe variable to set it instead of a command line option. This will allow:
|
fbdcee6
to
6b3c38e
Compare
This is a squash commit that was everything on the branch
Instead we will set it through a recipe variable.
It is passed through to the internal plotting functions, but not all of them use it yet.
6b3c38e
to
ca904e8
Compare
Fixes #1173
Contribution checklist
Aim to have all relevant checks ticked off before merging. See the developer's guide for more detail.