Adjust nz calculation to match EMOD3D changes in generate_3d_model.py - #60
Open
andy22b wants to merge 2 commits into
Open
Adjust nz calculation to match EMOD3D changes in generate_3d_model.py#60andy22b wants to merge 2 commits into
andy22b wants to merge 2 commits into
Conversation
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
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
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.
Hey team!
Just tried using this to get VS30 for some sites. I was getting an issue with the depth check in threshold.py, I used Claude to find it and have pasted the issue below. Just using the previous release (v2026.4.1) for now. I also notices there's been a change in syntax since one of the README examples was written.
Thanks heaps!
Nz offbyone issue · MD
generate_1d_profilesandgenerate_thresholdsfail for every station:nzoff-by-one vs.gen_full_model_grid_great_circleSummary
Since
e0dbb96(merged in #59, velocity_model_node_correction),gen_full_model_grid_great_circleexpectsnzto include the EMOD3D padding row, butgenerate_1d_profiles.pyandthreshold.pystill computenzwithout it. Every station therefore fails validation, independent of coordinates, extent or spacing.Released version
2026.7.1on PyPI contains the commit, so a freshpip install velocity-modellingreproduces this on the first station.Reproduction
Actual
Expected
A profile is generated.
Cause
velocity_modelling/geometry.py:1095validates against:The
+ 1was added ine0dbb96to matchgenerate_3d_model.py(~L197-203), which adds a padding row because EMOD3D does not read the last layer of the velocity model. That commit touchedgeometry.pyonly.The other two callers that build a mesh through the same function still use the un-padded form:
velocity_modelling/scripts/generate_1d_profiles.py~L586:velocity_modelling/threshold.py:602:So
int(2.0 / 0.01 + 0.5) = 200on the caller side against201in the validator. This is not a floating-point edge case —2.0 / 0.01is exactly200.0.The
depth_valuesbranch ingenerate_1d_profiles.pyis affected too:nzis computed the same way with theh_depth = 1.0placeholder beforegen_full_model_grid_great_circleis called, andgm.nzis only overwritten afterwards.Affected versions
e0dbb96v2026.4.1and earlierv2026.07.1(current PyPI2026.7.1)Suggested fix
The padding row is an EMOD3D output requirement, not a property of the grid geometry, so hard-coding
+ 1in the shared validator arguably pushes an output-format concern intogeometry.py. Two options:nz_expectedtoint((zmax - zmin) / h_depth + 0.5)and keep the+ 1ingenerate_3d_model.pyonly, where the EMOD3D rationale already lives.+ 1ingenerate_1d_profiles.pyandthreshold.py.Option 2 changes 1D profile output: the profile becomes endpoint-inclusive (0.00-2.00 km rather than 0.00-1.99 km for the example above), which may be desirable but is a behavioural change worth calling out in the changelog. Note also that
geometry.py:1130shifts the shallowest node down byh_depth / 4, so the first sample is at 2.5 m rather than 0 m.A regression test covering
generate_1d_profilesandgenerate_thresholdsend-to-end for a single station would catch this class of drift between the callers and the validator.