Stop storing idomain in pwnlayers/pwnlayers3 layer models - #59
Merged
Conversation
idomain is derived from layer thickness and goes stale on any later geometry edit (e.g. the 09pwnmodel2 lake carve via set_model_top), while nlmod recomputes it from thickness at package-build time and never reads the stored variable (nlmod PR #250 'calculate_idomain_on_the_fly'). Storing it only poisons the persisted model dataset with values that misrepresent the built model. Direction confirmed by nlmod maintainer on NHFLO/models#120. Both get_pwn_layer_model variants are covered: pwnlayers3 (09pwnmodel2) and the older pwnlayers (10vlotter, 16dwat). No code in nhflotools, nlmod, or the modelscripts reads ds['idomain']; all consumers call get_idomain(ds) fresh.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the durable half of NHFLO/models#120, per the nlmod maintainer's guidance there: nlmod moved to computing idomain on the fly (nlmod PR #250) precisely to avoid stale stored copies, so
get_pwn_layer_modelshould never put it indata_vars.pwnlayers3/layers.py: dropidomainfrom the returned dataset (09pwnmodel2 path).pwnlayers/layers.py: same for the older variant (10vlotter, 16dwat path).Verified before removal: no code in nhflotools, nlmod, or any modelscript reads the stored
ds['idomain']— every consumer (includingnlmod.gwf.disv) callsnlmod.dims.layers.get_idomain(ds)fresh from thickness, and both stored copies were themselves pureget_idomainoutput, so no information is lost. The models-side transitionaldrop_vars('idomain')guard (pr3 branch) remains valid and becomes a no-op once this merges.Test suite passes (18 passed). Merge order: independent of tools#58; before the models pr3 PR ideally, but safe in any order thanks to
errors='ignore'on the script-side drop.