[21Pt] PR - Re-work Inundation.py [WIP] #1392
Conversation
Updates from dev
…A-OWP/inundation-mapping into dev-probabilistic-inundation Merge divergent branch
…A-OWP/inundation-mapping into dev-probabilistic-inundation
…into dev Mergin remote repo updates
tools/inundate_gms_optimized.py
Outdated
|
|
||
| except NoForecastFound as exc: | ||
| if log_file is not None: | ||
| print(f"{hucCode},{branch_id},{exc.__class__.__name__}, {exc}", file=open(log_file, "a")) |
There was a problem hiding this comment.
opening a file here might not be a good idea. Python docs recommend explicitly closing opened files and not relying on garbage collection to properly flush and close the file.
There was a problem hiding this comment.
ya.. we use "with" for every io based write for log files.
Also.. just today, we went though a ton of our files looking more carefully if objects can be deleted earlier. We had a habit of loading a file and keeping it open the entire function/method even though we used it early on. Much of FIM is now showing memory issues so we are cleaning those up. Good to keep an eye for it here too. You might already be doing it but just a heads up on it. :)
tools/tools_shared_functions.py
Outdated
| """ | ||
|
|
||
| for obj in objects_to_delete: | ||
| del obj |
There was a problem hiding this comment.
Note that this only deletes the reference to the object, not the object itself. If the reference count is greater than 0 then the object will continue to live.
There was a problem hiding this comment.
Love it. Maybe add that little note that Ryan mentions right in the code. (Helps future users) Note that this only deletes the reference to the object, not the object itself. If the reference count is greater than 0 then the object will continue to live.
There was a problem hiding this comment.
The attempt to manually manage memory here I think is misguided. Python is already doing most of this for you automatically. There is often no need to manually invoke expensive gc collections, though there are some exceptions.
There was a problem hiding this comment.
ya.. There are a few places it might help though (I think). We have some areas where we keep some very large and multiple datasets and lists open simultaneously. There are lots of places, a lot in other FIM Files and not so much here, we can drop objects after we need them, or pull out what we need to a subset object so we can let the bigger one be cleaned up earlier. Most of that is on a file by file, line by line basis. The addition of so many large dataset open at the same time are creating pretty big problems. (Granted.. this statement mostly applies to other FIM code files, and not so much this PR files. But bringing awareness to it helps. :) (I think). Code in multi processing have shown to be a bit slower on garbage collections and we have seen a few impacts on it.
There was a problem hiding this comment.
timing on the firing of the python garbage collection is async and minor delays can be seen on rare occasions.
This update is a work in progress pending acceptance criteria of a one to one comparison of the new method and the old one given a run of UAT. The caveat is that the old method will have to be run using a cutoff of a 1/10th of a foot or more as inundated as opposed to any value above 0 meters.
NOTE: Accidentally mixed up some older probabilistic scripts in here and will remove them from this PR shortly.
Substantive changes:
Additions
Changes
Removals
Testing
In Progress
Deployment Plan (For developer use)
How does the changes affect the product?
Issuer Checklist (For developer use)
You may update this checklist before and/or after creating the PR. If you're unsure about any of them, please ask, we're here to help! These items are what we are going to look for before merging your code.
[_pt] PR: <description>devbranch (the default branch), you have a descriptive Feature Branch name using the format:dev-<description-of-change>(e.g.dev-revise-levee-masking)devbranchpre-commithooks were run locally4.x.x.xMerge Checklist (For Technical Lead use only)