Closed claalmve closed 4 months ago
Is the notebook of interest in this PR? Is it ready for testing?
Is the notebook of interest in this PR? Is it ready for testing?
Yes! It's also ready for testing, sorry I didn't assign it. I added a description for context and should be ready for review!
Requesting that plot_double_WRF
be renamed to be specific to the kind of plot being produced, perhaps: plot_climate_response_WRF
This is perhaps a v5+ item to consider for this plot, but these kinds of comparison figures are a kind of climate response to a change signal: typically temperature delta (i.e., GWL) and change in precipitation (see Fig 6 here ). Going to leave a comment on the notebook PR on this as the "default" option for now.
The notebook adds median as an aggregation function option. I don't know the best scientific practice regarding this when both space and time dimensions need aggregation. Right now agg_lat_lon_sims
flattens the space dimension first through area-averaging (if a lat/lon box is provided), but agg_area_subset_sims
doesn’t. The results are likely similar, but I think the discrepancy should be resolved.
It's awesome that more variables are now supported by the tool. However, some variables in variable_descriptions.csv are not available here. Nor are they in the Select() panel. I am curious why, but the current variables work well.
It's awesome that more variables are now supported by the tool. However, some variables in variable_descriptions.csv are not available here. Nor are they in the Select() panel. I am curious why, but the current variables work well.
I can answer this one! Some variables are "turned off" in variable_descriptions.csv
because they're super uncommon to actually use. They're needed and accessible for other workflows but not necessary to include as a core main option for the time being!
The multi-cell lat/lon box in the notebook works well too. I like that inputs are validated. For future, a more robust lat/lon validation could be beneficial. It would be nice to check that the input lat/lon are selectable (within the limits supported etc.). I imagine some users would try integers in addition to floats, but suppose no one would put strings in the tuples. If the validation logic gets complex, maybe consider reusing existing ones created for Select.
Likewise, the years should be within limits supported.
plot_WRF
is giving unexpected unit and values:
plot_sims
is also breaking as the notebook gives it more arguments than it can take, but WRF seems to be more of interest now, so this could be addressed in a future PR.
plot_WRF
is giving unexpected unit and values:
I am also seeing this - and noted it in my original review
Out of curiosity, is there a reason plot_sims
is not plot_LOCA2
, considering the WRF equivalent is plot_WRF
?
This is very much not a needed by Thursday revision, but specificity and clarity should be considered in function names moving forward - presumably we will have future different kinds of plots for WRF and LOCA2 data
Description of PR
Summary of changes and related issue Implementing changes for V1.1 of visualizing WRF on an axis. Includes functionality to:
agg_lat_lon_sims
, instead of just the closest gridcell.Relevant motivation and context Informs users on what variables are available to use for agnostic tools. Also allows users to use agnostic tools across larger areas, spanning multiple gridcells.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Tested with various inputs inside of
agnostic_tools.ipynb
Checklist: