misc fixes #159

timcera · 2024-04-30T05:01:05Z

Fixes

Allow the HDF filename to be passed to main function instead of only io_manager instance. Having main only accept io_manager broke all ipython notebooks.
Removed "print(table)" from readUCI which I think was a debugging statement.

IPython Notebook Fixes

Edited some of the ipython notebooks that tried to find an element time-series with an old name, for example tried to find "SURO" instead of the new name "SURO_sum".
Edited some of the ipython notebooks so that filenames were case sensitive.

Deprecations

The append function is no longer available for DataFrames and changed to pd.concat form.
In pandas 3.0, to_hdf will require the "key" to be a keyword argument instead of a positional argument and made that change now to silence the deprecation message.

Format

Removed trailing spaces from some files.
Fixed not a multiple of 4 spaces indentation in at least one file.
Renamed files with spaces in their names, replacing spaces with "_".

Documentation

Additional edits to the README.rst.
Reshaped some docstrings into Numpy format.

Fixes * Allow the HDF filename to be passed to main function instead of only io_manager instance. Having main only accept io_manager broke all ipython notebooks. * Removed "print(table)" from readUCI which I think was a debugging statement. IPython Notebook Fixes * Edited some of the ipython notebooks that tried to find an element time-series with an old name, for example tried to find "SURO" instead of the new name "SURO_sum". * Edited some of the ipython notebooks so that filenames were case sensitive. Deprecations * The append function is no longer available for DataFrames and changed to pd.concat form. * In pandas 3.0, to_hdf will require the "key" to be a keyword argument instead of a positional argument and made that change now to silence the deprecation message. Format * Removed trailing spaces from some files. * Fixed not a multiple of 4 spaces indentation in at least one file. * Renamed files with spaces in their names, replacing spaces with "_". Documentation * Additional edits to the README.rst. * Reshaped some docstrings into Numpy format.

timcera · 2024-04-30T05:01:45Z

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:

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.
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

austinorr · 2024-04-30T13:48:53Z

pyproject.toml

@timcera can we remove the ‘exclude’ line below since the includes are explicit? I think we can also drop the ‘where’ line too since ‘here’ is the default. Works great in my testing to be less explicit here.

Sure. Now "import this" says explicit is better than implicit, just saying... 😄

I am in complete agreement with the 'import this' philosophy, that's actually why I suggested removing all the extra over-constrained instructions with an explicit include, bc it leads to confusion to have lots of dead code in our packaging. no-ops should not exist in a pyproject.toml.

Beautiful is better than ugly.
Explicit is better than implicit.
Simple is better than complex.
Complex is better than complicated.
Flat is better than nested.
Sparse is better than dense.
Readability counts.

^this is the case against having extra params here, not for it 😅

austinorr · 2024-04-30T13:56:14Z

publish.py

 subprocess.run(shlex.split("python3 -m build --wheel"), check=True)
-sdist = f"dist/{PKG_NAME}-{version}.tar.gz"


Does this script run now? Sdist is still in the list below, so I’d expect it to throw. I think we should publish sdist, so I’d revert this line change and keep it in the twine iterator.

we should probably also rm all files in ‘dist’ similar to how we clear out ‘build’ too, right? If we are publishing from CI only, then this doesn’t matter, but if we are publishing from desktop then it matters a lot.

let’s make sure top level build and dist are in gitignore (I think they are) and do a full clean of those folders via this script before publishing.

Good catch! I'll remove sdist from the list. I probably keep the loop and revisit adding sdist later. I'll push a commit in a bit...

About removing all file in "dist". I don't automatically remove with my own packages and it hasn't proven to be a problem. After a while they might stack up and I remove by hand just to clean things up. This script is really just a convenience for the couple of developers that would publish to PyPI. Probably should find another directory for it to make it less visible? That is something that I thought could wait.

austinorr · 2024-04-30T15:42:04Z

HSP2/__init__.py


 from HSP2.main import main
 from HSP2.mainDoE import main as mainDoE
 from HSP2.utilities import versions, flowtype
-from _version import __version__
+
+__version__ = version("hsp2")


TIL importlib judo. this is a nice solution.

timcera · 2024-04-30T18:20:38Z

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

austinorr · 2024-04-30T18:39:00Z

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 · 2024-04-30T20:02:08Z

Fine by me.

austinorr · 2024-04-30T23:37:04Z

@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 · 2024-05-01T02:57:50Z

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 · 2024-05-01T06:25:39Z

@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 · 2024-05-01T20:19:53Z

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:

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.

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 · 2024-05-01T20:28:30Z

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.

…s with undefined variables, as noted in PR #159.

austinorr · 2024-05-03T16:25:37Z

pyproject.toml

pin pandas<2.0 to defer refactoring all the df.append() calls. If this project installed clean as-is without pinning to <2.0, nothing can work since df.append() has been removed.

austinorr · 2024-05-03T16:28:37Z

HSP2tools/readUCI.py

-        df = df.append(Series(name=name1))        # add missing ids with NaNs
-    if df.isna().any().any():                      # replace NaNs with defaults
+        df = pd.concat(
+            [df, pd.Series(name=name1)], axis="columns"


wrong axis. old append does "Append rows of other to the end of caller," which is df.concat(other, axis='rows') or just df.concat(other)

This was referenced Apr 30, 2024

Pip install . & packaging checks #157

Closed

Add pytest to deps and CI #156

Merged

austinorr reviewed Apr 30, 2024

View reviewed changes

fixes for publish.py and pyproject.toml

9b73aac

PaulDudaRESPEC added a commit that referenced this pull request May 1, 2024

OXRX_Class.py -- explicitly set tcginv and len_ to avoid future issue…

059ea16

…s with undefined variables, as noted in PR #159.

austinorr reviewed May 3, 2024

View reviewed changes

PaulDudaRESPEC merged commit dc02461 into respec:develop May 3, 2024
0 of 3 checks passed

PaulDudaRESPEC mentioned this pull request May 7, 2024

Develop spec lean #161

Merged

rburghol pushed a commit to HARPgroup/HSPsquared that referenced this pull request May 7, 2024

Merge changes that were reverted from respec#159

3006ec7

aufdenkampe added this to the v0.12.0: Improved packaging, etc. milestone May 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc fixes #159

misc fixes #159

timcera commented Apr 30, 2024 •

edited

Loading

timcera commented Apr 30, 2024 •

edited

Loading

austinorr Apr 30, 2024

timcera Apr 30, 2024

austinorr Apr 30, 2024

austinorr Apr 30, 2024

timcera Apr 30, 2024

timcera Apr 30, 2024 •

edited

Loading

austinorr Apr 30, 2024

timcera commented Apr 30, 2024

austinorr commented Apr 30, 2024

timcera commented Apr 30, 2024

austinorr commented Apr 30, 2024 •

edited

Loading

timcera commented May 1, 2024

austinorr commented May 1, 2024

PaulDudaRESPEC commented May 1, 2024

PaulDudaRESPEC commented May 1, 2024

austinorr May 3, 2024 •

edited

Loading

austinorr May 3, 2024 •

edited

Loading

		subprocess.run(shlex.split("python3 -m build --wheel"), check=True)
		sdist = f"dist/{PKG_NAME}-{version}.tar.gz"

misc fixes #159

misc fixes #159

Conversation

timcera commented Apr 30, 2024 • edited Loading

timcera commented Apr 30, 2024 • edited Loading

austinorr Apr 30, 2024

Choose a reason for hiding this comment

timcera Apr 30, 2024

Choose a reason for hiding this comment

austinorr Apr 30, 2024

Choose a reason for hiding this comment

austinorr Apr 30, 2024

Choose a reason for hiding this comment

timcera Apr 30, 2024

Choose a reason for hiding this comment

timcera Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

austinorr Apr 30, 2024

Choose a reason for hiding this comment

timcera commented Apr 30, 2024

austinorr commented Apr 30, 2024

timcera commented Apr 30, 2024

austinorr commented Apr 30, 2024 • edited Loading

timcera commented May 1, 2024

austinorr commented May 1, 2024

PaulDudaRESPEC commented May 1, 2024

PaulDudaRESPEC commented May 1, 2024

austinorr May 3, 2024 • edited Loading

Choose a reason for hiding this comment

austinorr May 3, 2024 • edited Loading

Choose a reason for hiding this comment

timcera commented Apr 30, 2024 •

edited

Loading

timcera commented Apr 30, 2024 •

edited

Loading

timcera Apr 30, 2024 •

edited

Loading

austinorr commented Apr 30, 2024 •

edited

Loading

austinorr May 3, 2024 •

edited

Loading

austinorr May 3, 2024 •

edited

Loading