-
Notifications
You must be signed in to change notification settings - Fork 26
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
Improve Handling of Fill-Values in RAD_MAX Estimator #282
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #282 +/- ##
==========================================
+ Coverage 95.60% 95.74% +0.13%
==========================================
Files 62 62
Lines 3278 3381 +103
==========================================
+ Hits 3134 3237 +103
Misses 144 144 ☔ View full report in Codecov by Sentry. |
2e3de6e
to
b6b33c9
Compare
elif self.grid_dim == 2: | ||
target_simplex = self.triangulation.find_simplex(target_point) | ||
|
||
if (target_simplex == -1) and issubclass( |
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.
You are checking for BaseNearestNeighborSearcher
here twice, only the check on target_simplex
is added, but then you do the same in both cases.
So just checking for issubclass(BaseNearestNeighborSearcher
should be enough?
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.
target_simplex == -1
means target_point
outside grid, thus this checks for cases, where extrapolation will actually be applied and only in these cases the issubclass
check on self.extrapolator.__class__
yields the needed information. In the second case, thus target_simplex != -1
, this needs to check self.interpolator.__class__
.
Basically, this accounts for the possibility that the user mixes and uses NearestNeighbor for extrapolation and actual interpolation otherwise (and vice-versa) at the same time.
"Fill-value handling only supported in up to two grid dimensions." | ||
) | ||
|
||
# Early exit if a nearest neighbor estimation would be overwritten |
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.
Why can't we just check for nearest neighbor? Why this complex setup?
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.
That setup is to catch cases where the user decided to mix approaches and use e.g. NearestNeighbor for interpolation but an actual extrapolator for extrapolation or vice-versa. I'll extend the comment in code to make this more clear for the future.
b6b33c9
to
9b0dace
Compare
9b0dace
to
26a9d8b
Compare
In the current version, the RAD_MAX estimator does not handle fill-values and thus produces results that inter-/extrapolate between actual values and fill-values, yielding results that do not resemble the expected behavior in edge-cases (e.g. RAD_MAX values sharply rising after some energy, see Fig. below). This PR introduces some form of combinatoric edge-case handling in 1 and 2D grids.
rad_max_cuts_new.pdf