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

Return the edge closer to the target in _numeric_derivative #1355

Merged
merged 3 commits into from
Apr 8, 2024

Conversation

dachengx
Copy link
Collaborator

@dachengx dachengx commented Apr 6, 2024

Before you submit this PR: make sure to put all operations-related information in a wiki-note, a PR should be about code and is publicly accessible

What does the code in this PR do / what does it improve?

Try to fix #1354

The change is minor so I will not update the version of plugins.

Can you briefly describe how it works?

Like the MWE in #1354 listed, when

aftobs, aft, s1tot = 1.0, 0.18243408203125003, 2.0

The j_min and j_max are too close (j_min is 0, and j_max is 2.8076670357142592e-05) and the difference between their corresponding y0 and y1 is smaller than err=1e-7. Then before this PR, this line

will directly return j_max. But in principle, it should return the x which will give the y closer to the target. And in the MWE of #1354, it should return j_min.

Can you give a minimal working example (or illustrate with a figure)?

image
The vertical lines are j_min and j_max which are very close.

Please include the following if applicable:

  • Update the docstring(s)
  • Update the documentation
  • Tests to check the (new) code is working as desired.
  • Does it solve one of the open issues on github?

Notes on testing

  • Until the automated tests pass, please mark the PR as a draft.
  • On the XENONnT fork we test with database access, on private forks there is no database access for security considerations.

All italic comments can be removed from this template.

@dachengx dachengx marked this pull request as ready for review April 6, 2024 11:10
@coveralls
Copy link

coveralls commented Apr 6, 2024

Coverage Status

coverage: 91.37% (+0.02%) from 91.347%
when pulling cf74100 on fix_numeric_derivative
into 915183e on master.

Copy link
Collaborator

@yuema137 yuema137 left a comment

Choose a reason for hiding this comment

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

Thanks @dachengx. I looked into the code and found that the problem is caused by this edge case (see attached drawing). It happens when there is actually no root for pmf(j) =pmf(k). In the old version, the program should return x0 which is 0 and will be converted to np.nan by the following functions, but it returned non-zerox1 which made the following functions think there is a root.
While the current modification can solve this problem, I think it's better to also highlight the case when there is actually no root:

if abs(y1 - y0) < err:
     if abs(target-y0) > err * 2 (any safeguard will be good):
           return 0.0, 0.0, 0.0
image It doesn't really change the performance considering there is only one edge case when we cannot find the root. So feel free to add the above line or not.

@dachengx dachengx merged commit 02210f9 into master Apr 8, 2024
8 checks passed
@dachengx dachengx deleted the fix_numeric_derivative branch April 8, 2024 03:05
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.

Corner case of test_inverse_pdf failure
3 participants