Add option to specify equality predicate for compare_values and provide a more sensible default (IS)
#387
Conversation
GabrielKS
changed the title
compare_values and provide a more sensible defaultcompare_values and provide a more sensible default (IS)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #387 +/- ##
==========================================
+ Coverage 78.20% 78.28% +0.08%
==========================================
Files 71 71
Lines 5473 5484 +11
==========================================
+ Hits 4280 4293 +13
+ Misses 1193 1191 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
and provide a more sensible default.
GabrielKS
force-pushed
the
gks/compare_values_match_fn
branch
from
3a397f3 to
295e365
Compare
|
Okay, the interface should be backwards compatible now. |
| return compare_values(x, y; compare_uuids = compare_uuids, | ||
| exclude = union(exclude, [COMPARE_VALUES_SENTINEL])) | ||
| end | ||
| exclude = setdiff(exclude, [COMPARE_VALUES_SENTINEL]) |
There was a problem hiding this comment.
This approach is a little hacky but I am convinced something along these lines is the least bad way to accomplish the task without changing the interface at all (and it's temporary)
There was a problem hiding this comment.
(ask me what I learned about Core.kwcall today)
| @@ -1040,6 +1040,7 @@ function _try_time_series_metadata_by_full_params( | |||
| end | |||
|
|
|||
| function compare_values( | |||
| match_fn::Union{Function, Nothing}, | |||
There was a problem hiding this comment.
Wait, what is supposed to happen here is a user passes a nothing?
There was a problem hiding this comment.
We can't just do
compare_values(x, y; kwargs...) = compare_values(isequivalent, x, y; kwargs...)because we need to be able to keep track of the fact that the two-argument version was initially called so that an old two-argument version in PSY can be called where relevant (I think this was the backwards compatibility issue with my first attempt). So we do
compare_values(x, y; kwargs...) = compare_values(nothing, x, y; kwargs...)and define that nothing will eventually default to isequivalent if it isn't intercepted by a two-argument version. This is documented here:
InfrastructureSystems.jl/src/utils/utils.jl
Line 136 in 295e365
| compare_values(x, y; kwargs...) = compare_values(nothing, x, y; kwargs...) | ||
|
|
||
| # Get the default match_fn if necessary. Only call when you know you're done recursing | ||
| _fetch_match_fn(match_fn::Function) = match_fn |
There was a problem hiding this comment.
This is quite a complicated workaround... I understand it avoids code repetition but it makes the code harder to read
There was a problem hiding this comment.
Would
(isnothing(match_fn) ? isequivalent : match_fn)(a, b)be more readable than
_fetch_match_fn(match_fn)(a, b)?
There is a docstring at InfrastructureSystems.jl/src/utils/utils.jl Line 132 in 295e365 |
| compare_uuids = false, | ||
| exclude = Set{Symbol}(), | ||
| ) where {T} | ||
| function compare_values(match_fn::Union{Function, Nothing}, x::T, y::U; |
There was a problem hiding this comment.
@GabrielKS In what cases do we pass different types to this function? How can two values that are of different types be considered equivalent?
There was a problem hiding this comment.
We want for instance isequivalent(0, 0.0), both because it makes sense and for backwards compatibility. In a broader sense we want to impose minimal constraints on how match_fn works, if it wants to specify that things of different type don't match then it is free to do so (and the default does generally do this).
There was a problem hiding this comment.
Isn't the purpose of compare_values to "Recursively compares struct values."? We should always be comparing identical types. Are we ever calling it to test integers and floats?
Previously, at the leaves of
compare_values's recursion tree, it would mostly test equality using the==operator, sometimes theBase.isequalfunction. This PR lets a sophisticated user specify a custom equality predicatematch_fnto override that behavior, such as to test a more approximate equality on a system when it is exported to a certain format and reimported. It also standardizes the defaultmatch_fnto, which is
trueforNaN, NaN(unlike==) and for-0.0, 0.0(unlikeisequal). Is that a violation of SemVer?