Conversation
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Pull request overview
This PR targets issue #86 by adding NetCDF-4 deflate compression and explicit chunking to reduce standard output file sizes and improve time-series access patterns.
Changes:
- Add zlib/deflate compression and chunking to standard NetCDF outputs (notably 2D time×basin variables) and to custom NetCDF outputs.
- Change the standard-output NetCDF
timedimension fromNC_UNLIMITEDto a fixed length derived fromOptions.duration/Options.timestep. - Introduce NetCDF compression/chunking constants in
RavenInclude.hand some related formatting-only cleanups.
Reviewed changes
Copilot reviewed 5 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/StandardOutput.cpp | Adds deflate + chunking and makes standard-output time dimension fixed; updates NetCDFAddMetadata2D to apply compression/chunking. |
| src/CustomOutput.cpp | Adds deflate + chunking to custom NetCDF variables and introduces ComputeNumTimeSteps() to size time. |
| src/CustomOutput.h | Declares ComputeNumTimeSteps(). |
| src/RavenInclude.h | Adds NETCDF_DEFLATE_LEVEL and NETCDF_CHUNKSIZE_MB constants. |
| src/Makefile | Forces NetCDF compilation/linking on by default. |
| src/Reservoir.cpp / src/Reservoir.h | Whitespace/formatting-only changes. |
| src/ParsePropertyFile.cpp | Whitespace-only changes. |
| src/ChannelXSect.cpp | Comment whitespace-only change. |
| src/Assimilate.cpp | Whitespace-only change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ng. Add compression to other outputs (storage, rainfall)
|
Tested against the RavenPy test suite and no error. Bonus, the outputs from the test take half the disk space. |
|
@huard showing off my github ignorance today - I thought I had merged the main into deflate but I still see recent changes in main not included in the 'Files changed'. It seems like it only merged the conflicts I approved but not the unconflicted changes?? Can you tell me how to do this? There was a separate button for the other pull requests that made this easy, but not here...? |
analytophile
left a comment
There was a problem hiding this comment.
not sure why my merge with main didn't take, but it is still holding onto these features of the previous branch as if they were additions in deflate
There was a problem hiding this comment.
should revert to main
There was a problem hiding this comment.
should revert to main
| for (int p = 0; p < _nSubBasins; p++) { | ||
| if((_pSubBasins[p]->IsGauged()) && (_pSubBasins[p]->IsEnabled())){ | ||
| if (Options.assim_method==DA_ECCC){ASSIM<<","<<_aDAQadjust[p]<<" ";} | ||
| else {ASSIM<<","<<_aDAscale [p]<<" ";} |
|
|
||
| if(!overriding) | ||
| { | ||
| for(int n=0;n<_nQlatHist;n++) { |
| void SetGauged (const bool isgauged); | ||
| void Disable (); | ||
| void Enable (); | ||
| double ScaleAllFlows (const double &scale_factor, const bool scale_last, const double &tstep, const double &t); |
|
What happened is that I merged main, then reverted that merge because I couldn't compiile. I'll try to make a clean branch and open a new PR. |
Pull Request Checklist:
What kind of change does this PR introduce?
Compression is particularly useful with simulations that do not include q_obs. Without compression, we still need to store the full array of NaNs.
Does this PR introduce a breaking change?
Should not, but I'm going to run more tests.
Note that this PR is based off v4.12 because the main does not compile on my end, nor on the CI. This PR should not be merged before main is merged into this branch.