-
Notifications
You must be signed in to change notification settings - Fork 3
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 precipitation histogram to prognostic run report #1271
Conversation
@@ -229,16 +227,6 @@ def _assign_diagnostic_time_attrs( | |||
return diagnostics_ds | |||
|
|||
|
|||
def dump_nc(ds: xr.Dataset, f): |
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.
We switched to using the vcm.dump_nc
version of this a while ago, so this func is unused.
logger.info("Computing histograms for physics diagnostics") | ||
counts = xr.Dataset() | ||
for varname in prognostic.data_vars: | ||
count, bins = np.histogram( |
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.
I found using np.histogram
much faster (and simpler code-wise) than doing an xarray
groupby.
def plot_1d( | ||
run_diags: RunDiagnostics, varfilter: str, run_attr_name: str = "run", | ||
) -> HVPlot: | ||
def plot_1d(run_diags: RunDiagnostics, varfilter: str) -> HVPlot: |
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.
deleted unused argument run_attr_name
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 great. Apart from some minor comments below, I had some thoughts
- I was pretty confused about why the metric functions returned
xr.Dataset
rather than floats, before realizing that theto_dict
function looks at the units and values. It would be clearer if the metrics functions returned dataclasses with.value
and.units
attrs, so that we are passing the minimal amount of data around. - It would be great to enhance the visibility of the scalar metrics, so that they are amongst the first things we see. e.g. by showing a table instead of the histograms. The histograms are kind of useless and ugly IMO, especially for a single run report.
workflows/prognostic_run_diags/fv3net/diagnostics/prognostic_run/compute.py
Show resolved
Hide resolved
workflows/prognostic_run_diags/fv3net/diagnostics/prognostic_run/metrics.py
Outdated
Show resolved
Hide resolved
workflows/prognostic_run_diags/fv3net/diagnostics/prognostic_run/metrics.py
Show resolved
Hide resolved
yeah I agree the
ouch, poor histogram ;) It would be helpful to have the verification data on there, I agree. The example is particularly noisy since it's a short run. But yeah, a table showing the percentiles for each run would be quite useful. |
sorry. I didn't mean your histograms, i meant the bar charts with the metrics on the main page. The histograms are great! |
haha sounds good. I agree the bar charts are hard to parse. I'll work on adding a metrics table with the things we actually care about. |
Useful to show a histogram of precipitation on prognostic run reports. Example report HERE.
Significant internal changes:
process_diagnostics.html
page to the prog run report. Moved diurnal cycle there, and added plot of precipitation histogram.Sped up report test by only creating report for a single run (since no longer require >1 run for report generation).