-
Notifications
You must be signed in to change notification settings - Fork 117
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
Conversation
3263606
to
a36aa9a
Compare
ef69e24
to
c79912e
Compare
There was a problem hiding this 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?
c79912e
to
e8c7af6
Compare
* move is_same from GenericUtilitiesModule to new MathUtilModule * draft modified routine for approximate floating point equality * add unit tests comparing is_same with proposed approach
71be222
to
2b19bc5
Compare
Removed the older |
The
GenericUtilitiesModule
currently has a functionis_same()
which uses a symmetric relative comparison for floating point equality. Move it to newMathUtilModule
asGenericUtilitiesModule
is mostly simulation related and depends onSimVariablesModule
etc. Modify floating point equality routine and rename tois_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()
andnumpy.isclose()
. Introduce functionis_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 newis_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 zeroTimeSeries
andTimeArraySeries
time comparisonsGridSorting.f90
point_in_polygon()
could support fuzzy boundariesReference