Skip to content

Conversation

@wpbonelli
Copy link
Contributor

@wpbonelli wpbonelli commented Jul 30, 2025

While it's on the user to provide both release coordinates and cell IDs, for some time we have checked at release time that release coordinates fall within the specified cell. Alternative ways to specify release points are in the works in #1901 e.g. local coordinates, which would remove the redundancy, but so long as release points are in global coordinates and we don't do cell identification ourselves, the coordinate checks are a performance bottleneck when particle count is high.

Add a COORDINATE_CHECK_METHOD option to allow opting out. Checks on (by default) with eager, trust me I know what I'm doing mode with none.

Later, consider adding a lazy mode where validation happens at tracking time, this might save some time if we can reuse terms of other calculations for the checks, but contradicts "fail early" philosophy so should probably not become default. Need to test and get some performance numbers.

If in future we did start computing cell IDs (e.g. with something fast like the celltree algorithm) this option could just be ignored when cell IDs are not provided by the user. So one could decide whether to do it in preprocessing or let PRT handle it.


Checklist of items for pull request

  • Replaced section above with description of pull request
  • Added new test or modified an existing test
  • Ran ruff on new and modified python scripts in .doc, autotests, doc, distribution, pymake, and utils subdirectories.
  • Formatted new and modified Fortran source files with fprettify
  • Added doxygen comments to new and modified procedures
  • Updated develop.toml with a plain-language description of the bug fix, change, feature; required for changes that may affect users
  • Updated input and output guide
  • Removed checklist items not relevant to this pull request

@wpbonelli wpbonelli marked this pull request as ready for review July 30, 2025 13:42
@wpbonelli wpbonelli requested a review from aprovost-usgs July 30, 2025 13:42
Copy link
Contributor

@aprovost-usgs aprovost-usgs left a comment

Choose a reason for hiding this comment

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

Looks good. Excellent summary of the related considerations in the Conversation, btw

@wpbonelli wpbonelli merged commit c94c7a3 into MODFLOW-ORG:develop Jul 30, 2025
19 of 20 checks passed
@wpbonelli wpbonelli deleted the prt-coord-checks branch July 30, 2025 15:55
@wpbonelli wpbonelli added this to the 6.7.0 milestone Sep 18, 2025
@martclanor
Copy link
Contributor

I just noticed that the default attribute in the DFN is probably meant to be default_value. Unless maybe this is intended?

@wpbonelli
Copy link
Contributor Author

@martclanor thanks, this was a mistake. It's default in the new schema https://github.com/MODFLOW-ORG/modflow-devtools/blob/3ef75eb78a3496cdca3b29df7ec79331c4743a62/modflow_devtools/dfn/parse.py#L148 but the DFN file should still use default_value, I guess I was living in the future 😅

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.

3 participants