respec / HSPsquared

Hydrologic Simulation Program Python (HSPsquared)
GNU Affero General Public License v3.0
43 stars 17 forks source link

misc fixes #159

Closed timcera closed 1 month ago

timcera commented 2 months ago

Fixes

IPython Notebook Fixes

Deprecations

Format

Documentation

timcera commented 2 months ago

I was sitting on these changes because I couldn't find solutions to two problems. So thought to put this in a pull request for review and maybe someone will be able to figure them out.

Another reason to get this out there, is for the next two weeks or so I won't be able to work on this and I didn't want to slow progress down.

"pytest" works to test the new PWATER and IWATER tests. They both pass.

THE PROBLEMS:

  1. Right now, using python 3.10 and a "pip install ." with the code in this pull request, the "readUCI" function in tests/test05/HSP2results/CompareHSP2.ipynb will only bring in the PERLND operation. IMPLND and RCHRES are ignored. No errors. I can't figure it out.

  2. In tests/test10/HSP2results/TEST10_hsp2_compare.ipynb, when it is calculating RQUAL I get the error:

...
File [~/anaconda3/envs/hsp2_310/lib/python3.10/site-packages/HSP2/RQUAL.py:225](http://localhost:8888/home/tim/anaconda3/envs/hsp2_310/lib/python3.10/site-packages/HSP2/RQUAL.py#line=224), in rqual(io_manager, siminfo, uci, uci_oxrx, uci_nutrx, uci_plank, uci_phcarb, ts, monthdata)
    218         ts['BINV'] = initm(siminfo, ui_plank, binvfg, 'MONTHLY[/BINV](http://localhost:8888/BINV)', binv_init)
    220 #---------------------------------------------------------------------
    221 # initialize & run integerated WQ simulation:
    222 #---------------------------------------------------------------------
    224 (err_oxrx, err_nutrx, err_plank, err_phcarb) \
--> 225     = _rqual_run(siminfo_, ui, ui_oxrx, ui_nutrx, ui_plank, ui_phcarb, ts)
    227 #---------------------------------------------------------------------
    228 # compile errors & return:
    229 #---------------------------------------------------------------------
    231 (errors, ERRMSGS) = _compile_errors(NUTFG, PLKFG, PHFG, err_oxrx, err_nutrx, err_plank, err_phcarb)

SystemError: CPUDispatcher(<function _rqual_run at 0x7f67104b8ee0>) returned NULL without setting an exception
timcera commented 2 months ago

I can make the shift to a "src/hsp2/..." package tonight that was talked about in #157.

austinorr commented 2 months ago

I can make the shift to a "src/hsp2/..." package tonight that was talked about in #157.

We should let @PaulDudaRESPEC catch up on merging some of these PR's and/or asking you or I to rebase our already open PRs before we go ahead and change literally every file in the project.

Also @rburghol has significant WIP that we should consider helping to support integrating before making major refactoring changes.

@PaulDudaRESPEC This is where an order of operations and/or roadmap with a todo list would help, otherwise we'll end up constantly overwriting each other and working on the same things in parallel. Also, we should temporarily disable the conda github action -- it never completes.

timcera commented 2 months ago

Fine by me.

austinorr commented 2 months ago

@timcera is this PR otherwise ready to merge in your opinion? If so, i'll rebase mine on this one, and @PaulDudaRESPEC can work the backlog a bit so we can all get the benefit of the improvements you've made here. Thank you!

timcera commented 2 months ago

Depends. Still have the two problems that I mentioned in the second comment. In terms of git repository management it might be best to get this one merged and then address those problems in a separate pull request. Up to @PaulDudaRESPEC on path forward.

austinorr commented 2 months ago

@timcera i agree, let’s merge this and move on of your only issues are in the ./tests dir. tests are probably due for a refactor anyway so that they work with pytest.

@PaulDudaRESPEC looks like this is blessed by Tim and ready for review & merge into dev

PaulDudaRESPEC commented 2 months ago

I was sitting on these changes because I couldn't find solutions to two problems. So thought to put this in a pull request for review and maybe someone will be able to figure them out.

Another reason to get this out there, is for the next two weeks or so I won't be able to work on this and I didn't want to slow progress down.

"pytest" works to test the new PWATER and IWATER tests. They both pass.

THE PROBLEMS:

  1. Right now, using python 3.10 and a "pip install ." with the code in this pull request, the "readUCI" function in tests/test05/HSP2results/CompareHSP2.ipynb will only bring in the PERLND operation. IMPLND and RCHRES are ignored. No errors. I can't figure it out.
  2. In tests/test10/HSP2results/TEST10_hsp2_compare.ipynb, when it is calculating RQUAL I get the error:
...
File [~/anaconda3/envs/hsp2_310/lib/python3.10/site-packages/HSP2/RQUAL.py:225](http://localhost:8888/home/tim/anaconda3/envs/hsp2_310/lib/python3.10/site-packages/HSP2/RQUAL.py#line=224), in rqual(io_manager, siminfo, uci, uci_oxrx, uci_nutrx, uci_plank, uci_phcarb, ts, monthdata)
    218       ts['BINV'] = initm(siminfo, ui_plank, binvfg, 'MONTHLY[/BINV](http://localhost:8888/BINV)', binv_init)
    220 #---------------------------------------------------------------------
    221 # initialize & run integerated WQ simulation:
    222 #---------------------------------------------------------------------
    224 (err_oxrx, err_nutrx, err_plank, err_phcarb) \
--> 225   = _rqual_run(siminfo_, ui, ui_oxrx, ui_nutrx, ui_plank, ui_phcarb, ts)
    227 #---------------------------------------------------------------------
    228 # compile errors & return:
    229 #---------------------------------------------------------------------
    231 (errors, ERRMSGS) = _compile_errors(NUTFG, PLKFG, PHFG, err_oxrx, err_nutrx, err_plank, err_phcarb)

SystemError: CPUDispatcher(<function _rqual_run at 0x7f67104b8ee0>) returned NULL without setting an exception

@timcera , I've got some helpful info to share...

With regard to problem 1, Test05 only runs PERLND, not IMPLND or RCHRES, so I don't think that is a concern.

As for problem 2, I see an issue with the recent changes to readUCI.py. At line 88 in fix_df, we've gone to using concat instead of append, but the new way of doing it with concat is not equivalent to the old append. In this case concat is adding a new column to the df, where append was adding a new row. I reproduced the error, changed it back to the old way, and the old way worked. I don't see what the appropriate fix is moving forward, but I think it will be helpful to point this out.

PaulDudaRESPEC commented 2 months ago

I got Test10 to run to completion using this code by @timcera and it matches what I believe to be the correct output values, but not without fixing a few things down in OXRX_class.py. It seems previous versions didn't have a problem with sending undefined variable names into RQUAL if those variables were never used, but now I see it triggering an error. So I've modified OXRX_class.py and will commit my changes to develop where I explicitly declare these variables.