Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: introduce MathUtilModule, move is_same, add tests #1446

Merged
merged 13 commits into from
Dec 13, 2023

Conversation

wpbonelli
Copy link
Contributor

@wpbonelli wpbonelli commented Nov 16, 2023

The GenericUtilitiesModule currently has a function is_same() which uses a symmetric relative comparison for floating point equality. Move it to new MathUtilModule as GenericUtilitiesModule is mostly simulation related and depends on SimVariablesModule etc. Modify floating point equality routine and rename to is_close().

Motivation

Relative comparisons get more sensitive to difference near zero holding epsilon equal, and if either value is 0, equality is only reported with epsilon >= 1. This can cause difficulties if it's unknown whether values to compare will be close to 0.

A common approach is to fall back to an absolute comparison like Python's math.isclose() and numpy.isclose(). Introduce function is_close() doing this.

Python stdlib math and numpy use different abstol defaults: math.isclose() sets the default absolute tolerance to zero, motivation that there is no universally reasonable default. The new is_close() here follows this but I'm not sure that's the best way.

An option is also provided to toggle asymmetric comparison, e.g. with a known reference value to which relative difference should be scaled

Places (near-)zero comparisons are or might be made:

  • utils/mf5to6/src/Preproc/ObsBlock.f90 checks explicitly against zero
  • TimeSeries and TimeArraySeries time comparisons
  • coordinate comparisons in GridSorting.f90
  • point_in_polygon() could support fuzzy boundaries

Reference

@wpbonelli wpbonelli force-pushed the approx-fp-eq branch 3 times, most recently from ef69e24 to c79912e Compare December 7, 2023 13:58
@wpbonelli wpbonelli marked this pull request as ready for review December 7, 2023 14:19
Copy link
Contributor

@langevin-usgs langevin-usgs left a comment

Choose a reason for hiding this comment

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

@wpbonelli, this looks good to me. If we switch to is_close() do we still need is_same()? And I'm sure it's fine, but maybe in TestMathUtil we should explicitly verify that comparison with DNODATA works as this is used in csub?

@wpbonelli wpbonelli added code refactor Nonfunctional changes maintenance Sprucing up the code labels Dec 11, 2023
@wpbonelli
Copy link
Contributor Author

wpbonelli commented Dec 13, 2023

Removed the older is_same() as it is no longer needed. Since the default abstol is zero, this should behave identically. It may be worth revisiting some uses and considering whether to use a nonzero abstol. Also added some test cases comparing with DNODATA as used in gwflak, I only saw one double precision comparison (with 0) in csub.

@wpbonelli wpbonelli merged commit c376899 into MODFLOW-USGS:develop Dec 13, 2023
15 checks passed
@wpbonelli wpbonelli deleted the approx-fp-eq branch December 13, 2023 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code refactor Nonfunctional changes maintenance Sprucing up the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants