Conversation
davemfish
left a comment
There was a problem hiding this comment.
Thanks, @megannissel , I left a few comments recapping some of what we discussed yesterday.
…ather than creating another copy of the AOI (natcap#2321)
…o reflect these conditions (natcap#2321)
There was a problem hiding this comment.
Thanks for all the hard work on this @megannissel . The report is looking great. I have a few suggestions for your consideration.
Please note that I did not review changes in cecba2e but I will whenever you pass the PR back to me. Thanks!
| n_cols = 3 | ||
|
|
||
| if small_plots: | ||
| n_cols += 1 |
There was a problem hiding this comment.
This is kind of neat if that's all it takes to make smaller plots! We'll probably want some snapshot tests that try different layouts with this option.
There was a problem hiding this comment.
I added some snapshot tests in 5e78639 and opened a PR in the test data repo with the new images.
There was a problem hiding this comment.
Cool, looks like it works well. It's ideal when the image ends up close to 1200 px wide, since that's what we specify as a target to fit nicely in our templates. So that first image where there are 2 columns in the small plot layout, it could stand to be larger overall. But I haven't thought about the best approach. We can always make an issue an improve on it later.
…nified colorbar for raster facet plots (natcap#2321)
… for linked chart+map (natcap#2321)
…cription in csv output spec (natcap#2321)
…ssion mfd and d8 cases (natcap#2321)
| imshow_kwargs['norm'] = transform | ||
| imshow_kwargs['interpolation'] = 'none' | ||
| cmap = COLORMAPS[dtype] | ||
| cmap = plt.get_cmap(config.colormap if config.colormap else COLORMAPS[dtype]) |
There was a problem hiding this comment.
This change is a defensive precaution. I noticed line 408 accesses cmap.N, which is fine at the moment because it's inside a conditional where (currently) cmap will always be a colormap object. But to help prevent bugs in the future (especially as we expand custom colormap support), it seems wise to always get the object. And plt.get_cmap() takes either a string or a Colormap object, so worst-case scenario it just returns the object that was passed to it.
There was a problem hiding this comment.
Cool, good idea. Another option might be to convert strings to a colormap object in RasterPlotConfig, either with a BeforeValidator or __post_init__, I forget exactly what the approach would be.
There was a problem hiding this comment.
Oh, I like this idea a lot! Looks like we can add this to the existing RasterPlotConfig.__post_init__
| """For highly skewed data, a transformation can help reveal variation.""" | ||
| title: str | None = None | ||
| """An optional plot title. If ``None``, the filename is used.""" | ||
| colormap: Optional[Union[str, object]] = None |
There was a problem hiding this comment.
@claire-simpson, related to #2447 -- RasterPlotConfig now accepts either the string name of a matplotlib colormap or a Colormap object. I've also renamed this attribute colormap, per Dave's suggestion!
There was a problem hiding this comment.
Instead of object can we use matplotlib.colors.Colormap? https://matplotlib.org/stable/api/_as_gen/matplotlib.colors.Colormap.html#matplotlib.colors.Colormap
Also, we don't necessarily need to support Python versions before 3.10, so instead of Optional and Union I think we can say str | Colormap | None = None.
|
|
||
|
|
||
| def raster_inputs_summary(args_dict): | ||
| def raster_inputs_summary(args_dict, model_spec): |
There was a problem hiding this comment.
These changes address the issue documented in #2440
|
Back to you, @davemfish! I think one of the biggest outstanding questions is still whether the quickflow + baseflow + precipitation values should be provided in m3/second or m3/month. Rafa deferred to Adrian, who previously expressed a preference for m3/month (but hasn't chimed back in yet). I made the change, but would like to get final confirmation (perhaps when I send out an updated version of the report for review). |
…bers (to support consistent month indexing) (natcap#2321)
davemfish
left a comment
There was a problem hiding this comment.
@megannissel Just a few minor suggestions here.
Also feel free to update the Makefile to point to the test data revision on your branch; we don't necessarily need to wait until that is merged.
There's also a ReadTheDocs failure which is probably due to this:
/home/docs/checkouts/readthedocs.org/user_builds/invest/conda/2439/lib/python3.11/site-packages/natcap/invest/reports/vector_utils.py:docstring of natcap.invest.reports.vector_utils.get_geojson_bbox:13: WARNING: Bullet list ends without a blank line; unexpected unindent. [docutils]
| """For highly skewed data, a transformation can help reveal variation.""" | ||
| title: str | None = None | ||
| """An optional plot title. If ``None``, the filename is used.""" | ||
| colormap: Optional[Union[str, object]] = None |
There was a problem hiding this comment.
Instead of object can we use matplotlib.colors.Colormap? https://matplotlib.org/stable/api/_as_gen/matplotlib.colors.Colormap.html#matplotlib.colors.Colormap
Also, we don't necessarily need to support Python versions before 3.10, so instead of Optional and Union I think we can say str | Colormap | None = None.
| n_cols = 3 | ||
|
|
||
| if small_plots: | ||
| n_cols += 1 |
There was a problem hiding this comment.
Cool, looks like it works well. It's ideal when the image ends up close to 1200 px wide, since that's what we specify as a target to fit nicely in our templates. So that first image where there are 2 columns in the small plot layout, it could stand to be larger overall. But I haven't thought about the best approach. We can always make an issue an improve on it later.
| imshow_kwargs['norm'] = transform | ||
| imshow_kwargs['interpolation'] = 'none' | ||
| cmap = COLORMAPS[dtype] | ||
| cmap = plt.get_cmap(config.colormap if config.colormap else COLORMAPS[dtype]) |
There was a problem hiding this comment.
Cool, good idea. Another option might be to convert strings to a colormap object in RasterPlotConfig, either with a BeforeValidator or __post_init__, I forget exactly what the approach would be.
| ) | ||
|
|
||
| chart = attr_map | combined_chart | ||
| legend_config = vector_utils.LEGEND_CONFIG |
There was a problem hiding this comment.
We should consider making a copy of the global variable before modifying it just in case LEGEND_CONFIG is used elsewhere.
I feel like we're good to proceed with the m3/month 👍 |
| os.path.join(args['workspace_dir'], 'aggregated_results_swy.shp'), | ||
| agg_results_csv_path) | ||
|
|
||
| def test_climate_zones(self): |
There was a problem hiding this comment.
This is only tangentially related to this PR, but when I updated the SWY model to consistently index monthly rasters, I accidentally introduced a bug that I only caught because one of the sets of sample data I've been using for report demo purposes includes a monthly climate zones raster and table as inputs. It seemed like a good reason to add a test covering this use-case.
…um plots; point Makefile to invest-test-data revision (natcap#2321)
… qb + vri_sum plot color scheme (natcap#2321)
| self.caption = f'{self.title}:{self.spec.about}' | ||
|
|
||
| self.colormap = plt.get_cmap(self.colormap if self.colormap | ||
| else COLORMAPS[self.datatype]) |
Description
Fixes #2321
This PR adds a reporter module and associated template for the Seasonal Water Yield model. In addition to including several elements seen in other reports (plotted output and input rasters, tables of raster statistics, and plots of values in the model's aggregate vector output), this report introduces a new element: an interactive map of the AOI linked to a chart of monthly average quickflow, baseflow, and precipitation values for each feature. These charts were requested by the model maintainers during an initial meeting about the contents of the report.
Since these charts are created from data that is derived from model outputs but not pulled from the directly, a new data source was required. This has taken the form of a CSV, which contains these monthly average quickflow, baseflow, and precipitation values by month, by feature in the AOI. After discussion with Dave, we decided that it would make the most sense for the model's
executefunction to create this CSV output, rather than creating it within the reporter. Since we think it would be beneficial to provide this data source to users to inspect, it makes sense to be able to describe the CSV within the model spec.A few additional noteworthy changes introduced by this PR:
vector_utils.created_ifcondition added, to reflect that they are only created whenuser_defined_local_recharge=False.A prototype of the report can be found here.
Additionally, a version of the report when
user_defined_local_rechargeisTrue(and the model therefore skips generating all of the quickflow rasters entirely) can be found here. Note that this version of the report is sparse; there simply aren't as many model inputs or outputs in this case.Related PRs:
RasterPlotFacetsTests: invest-test-data #43Checklist