Skip to content

Clean Merge for Develop class state - #228

Open
rburghol wants to merge 7 commits into
respec:developfrom
HARPgroup:develop-class-smerge
Open

rburghol wants to merge 7 commits into
respec:developfrom
HARPgroup:develop-class-smerge

Conversation

@rburghol

Copy link
Copy Markdown
Contributor

Replaces #224 after integrating recent UCI changes

This PR includes an overhaul to the model state system, leveraging a new object-oriented version of state, which accomplishes the following:

  • It streamlines the number of arguments passed to functions, so now, the arg state takes the place of state_ix, dict_ix, ts_ix, ...
  • Declarations of the base containers for state attributes is moved outside of the function, living directly in the state.py file, which allows these to be cached, resulting in a huge improvement in performance (recent branches had seen start-up time double due to less optimal numba caching).
  • An example of using dynamic equations has been added in tests/testcbp/HSP2results/ to test a 500 equation addon simulation (Use: cp PL3_5250_0001.json.manyeq PL3_5250_0001.json to test).
  • Some cleanup of arguments in function like hydr() and sedtrn() to eliminate the argument io_manager as this is no longer used (I think this has been due for a while, I don't think it is due to any changes that I recall making to the code but it seems like a cleaner approach)

Comment thread src/hsp2/hsp2/om.py Outdated
# This sets up the state entries for all state compatible HSP2 model variables
# print("STATE initializing contexts.")
for _, operation, segment, delt in opseq.itertuples():
if operation != "GENER" and operation != "COPY":

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be:

if operation not in ["GENER", "COPY", "PLTGEN", "DISPLY"]:

Or maybe (I think these are the operations that apply here, but ?):

if operation in ["RCHRES", "PERLND", "IMPLND"]:

I think this is the current test failure.

I read in "PLTGEN" and "DISPLY" operations now, even though unused by HSP2. My idea was to have complete interoperability between parameter stores (HDF5, UCI, and CSV).

@rburghol

Copy link
Copy Markdown
Contributor Author

Thanks @timcera -- checking this out now!

@rburghol

Copy link
Copy Markdown
Contributor Author

Thanks @timcera -- this definitely fixed my initial problem, though now I have some results differences that I have to sort out, I assume they are unrelated to the improvements that you've made to the UCI parser.

@rburghol

Copy link
Copy Markdown
Contributor Author

@timcera - I went back to basics on this to try and track my errors. I am having trouble importing UCI's with the latest develop branch, and this error FileNotFoundError: [Errno 2] No such file or directory: '/test10.wdm' is occurring with develop and with this branch here as well. I am assuming something that I am doing wrong, but can't figure it out. Can you test on this for me, full transcript for what I did:

git checkout develop
git pull origin develop
rm -Rf build
python3 -m pip install .
cd tests/test10/HSP2results
rm test10.h5
hsp2 import_uci test10.uci test10.h5

An I get FileNotFoundError: [Errno 2] No such file or directory: '/test10.wdm'

@timcera

timcera commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

Sorry that was me. Fixed in #229.

@rburghol

Copy link
Copy Markdown
Contributor Author

Thanks @timcera for the quick fix! Any clue how this passed the tests on the develop branch? It seems like it should bork in the testing env, but it looks like it made it through, perhaps there is an .h5 file in the repo, and thus, the test can proceed?

@timcera

timcera commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

The tests passed because they use a directory path to the UCI file, something like "tests/test10/HSP2results/test10.uci" instead of "test10.uci". When given "test10.uci" when you are in the directory the previous code found the parent directory as "" which meant that when it went to look for the WDM files (which are relative paths to the UCI) was looking for "/test10.wdm" instead of "test10.wdm". The pathlib Path function handles all of that without problem, and supports cross-platform path manipulations. I don't know why I didn't use it before.

@timcera

timcera commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

The fix in #229 isn't the cause of these test failures. I don't know what is causing this issue. Could it be something different in the way I am parsing and creating the Special Action tables that is different than what you were working with?

@rburghol

Copy link
Copy Markdown
Contributor Author

Thanks @timcera that makes sense! The tests are still failing now, but in the value comparison phase. Which is equally mystifying, but at least it's something! The tests in question all seem to be related to the differences in sediment induced by the special actions tests, so, that makes me think I broke something :). Even though the same code previously passed, perhaps I goofed something up when bringing that older code into the more recent develop branch. I'll try to sort this out.

Thanks again!

@timcera

timcera commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

If you find that it is something in the UCI parsing code, I would be glad to handle the fix.

Comment thread src/hsp2/hsp2/om.py Outdated
@rburghol

rburghol commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

@timcera -- apologies for my delay in getting back to you, your suggested fix did the trick if operation in ["RCHRES", "PERLND", "IMPLND"]: -- and now all tests pass. I thought I had messaged about this last week when we implemented it, and seems like I never pressed send.

I had hoped that the generic for _, operation, segment, delt in opseq.itertuples(): would be digestable by the system, but I was too ambitious.

@PaulDudaRESPEC - this PR is ready to merge unless you or Tim have reservations. It adds very little, but streamlines the code a bit, and I think that will facilitate better readability in the short term, and easier migrateability to the new looping approach when we get there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants