Add test script to process nwm forcing into ngen input#26
Open
jameshalgren wants to merge 13 commits intoAlabamaWaterInstitute:mainfrom
Open
Add test script to process nwm forcing into ngen input#26jameshalgren wants to merge 13 commits intoAlabamaWaterInstitute:mainfrom
jameshalgren wants to merge 13 commits intoAlabamaWaterInstitute:mainfrom
Conversation
Contributor
Author
groutr
reviewed
Dec 22, 2022
| ): | ||
|
|
||
| reng = "rasterio" | ||
| _xds = xr.open_dataset(folder_prefix.joinpath(file_list[0]), engine=reng) |
Contributor
There was a problem hiding this comment.
maybe open the file handles up first, then just use first filehandle to extract _xds. Then everything gets closed at the end.
Contributor
Author
There was a problem hiding this comment.
Maybe. We need the specific raster engine to get the right behavior from the _xds.U@D.rio.transform() line below, which we don't have in the later call...
See
_xds = xr.open_dataset(folder_prefix.joinpath(file_list[0]), engine=reng)vs.
filehandles = [xr.open_dataset("data/" + f) for f in file_list]
groutr
reviewed
Dec 22, 2022
| for f in filehandles: | ||
| cropped = xr_read_window(f, winslices, mask=mask) | ||
| stats.append(cropped.mean()) | ||
| [f.close() for f in filehandles] # Returns None for each file |
Contributor
There was a problem hiding this comment.
I did this for convenience. You could make this a for-loop for clarity
for f in filehandles:
f.close()
Contributor
Author
There was a problem hiding this comment.
I think the list comprehension is plenty readable... and more compact. Good enough?
groutr
reviewed
Dec 22, 2022
983aa5b to
fc67d18
Compare
1245428 to
e8caaf9
Compare
Contributor
Author
|
Moved todo to main comment. |
Contributor
Author
|
@mgdenno @arpita0911patel -- Let's discuss |
Contributor
Author
|
ping @karnesh |
Contributor
Author
1ab1532 to
3dc7705
Compare
added 9 commits
March 8, 2023 12:29
eb93ee1 to
2dc42b5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Working on this with respect to #24.
This is ready for at least initial review. The main script,
process_nwm_forcing_to_ngen.pyhas four functions which all produce the same thing -- a dataframe with the right information to provide forcing to the ngen CFE model in the form of precipitation inputs for each basin in a geopackage file with basin polygons from the ngen hydrofabric. In the test script, there is a primitive method included that creates a dictionary using an extremely slow technique, but one which is very clear to follow. (The test script, by the way, is not a canonical test script -- just amainfunction that runs a few example uses.)Work remaining might include the following:
--enable-parallelflag or here - possibly change to access pattern)Three levels of review are helpful:
A. Is a utility to convert NWM output to NGEN basins useful?
B. Who else should be tagged?
C. Deep review (code, methods, etc.)
How to test:
Download a set of test data files to use with
test_process_nwm_forcing_to_ngen.py:UpdateSee below...Times are as follows for tests with 3 files and 200 features, then 30 files and 2000 features, respectively:
There is a warning returned. Perhaps the processes are closing before the files close...
Update
With the latest commits, the fastest method is the "new" way with reversed loops, especially with a large number of files/timestamps. To produce a 30 hour input for 2000 basins takes between 220 and 250 seconds:
200 basins with 240 files:
A 10-day hourly input of rainfall (240 hours == one medium_range forecast) for 2000 basins in the Southeast W hydrofabric region took 30 minutes:
All execution times reported here from tests performed on a mac M1 Max with 64 Gb RAM.