search

NCAR / wrf_hydro_py

Python API for the WRF-Hydro model
59 stars 43 forks source link

Collect NWM datasets using xarray and dask #159

Closed jmccreight closed 5 years ago

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 337

Changes Missing Coverage Covered Lines Changed/Added Lines %
wrfhydropy/core/ioutils.py 27 153 17.65%
<!-- Total: 31 157 19.75% -->
Totals Coverage Status
Change from base Build 327: -4.6%
Covered Lines: 2156
Relevant Lines: 2475
💛 - Coveralls
jmccreight commented 5 years ago

I kept joe's old routine as it was used in testing (both in this repo and in the model repo).

I've added a new, better approach but have not reconciled it with the new one. I have not added tests for it. I'm open to creative ideas here...

I'm going to improve the ensemble/dart collect and then get more serious about merging this PR.

jmccreight commented 5 years ago

The DART dataset merge is now available and working great.

We could have a philosophical debate about if these routines belong in wrf_hydro_py unless we are specifically using them with the API. I'm guessing we eventually will. I would also argue that they are so important that they should be included regardless.

For the moment there are no tests for these collects. I feel OK about that until we do start using them with the API.

Also, the open_nwm_dataset, open_dart_dataset, and open_wh_dataset should be not too difficult to combine and thereby achieve significant reduction in code through reuse.

I would like to merge these and work towards combining and use with the API supported by testing over the next few months. Does that seem acceptable?

jmccreight commented 5 years ago

I'd hoped to fix those docstrings, I will try to get that done soon. Like I said, until we use it in the API I'm fine not testing it. I did write some tests for translating NWM forcing to LDASIN forcing that actually pull operational data (names) and work on those. We could do something similar for testing the open_nwm_dataset. Though the band width might be too much. Maybe we should generate dummy data in the test and merge it.