vp-comparefit issue, due to arclength errors by comane · Pull Request #1681 · NNPDF/nnpdf

comane · 2023-02-27T21:34:26Z

@Zaharid , @enocera Issue with vp-comparefit due to arclength function.
my guess is that the problem arises from these lines of the code within validphys.arclength.plot_arc_lengths:
yvalues = arclengths.stats.central_value()
ylower, yupper = arclengths.stats.errorbar68()
ylower = yvalues - ylower
yupper = yupper - yvalues
Some of the replicas might be shorter than the central replica. The problem is thus probably fixed by taking the absolute value of the difference.

Another possible issue might be in validphys.arclength.arc_lengths, namely, in
res += integrate.simps(1 + np.square(fdiff), ixgrid[1][1:])
It looks like a square root is missing here:
res += integrate.simps(np.sqrt(1 + np.square(fdiff)), ixgrid[1][1:])

Zaharid · 2023-02-27T22:11:10Z

@comane thanks for this!

@Zaharid , @enocera Issue with vp-comparefit due to arclength function. my guess is that the problem arises from these lines of the code within validphys.arclength.plot_arc_lengths: yvalues = arclengths.stats.central_value() ylower, yupper = arclengths.stats.errorbar68() ylower = yvalues - ylower yupper = yupper - yvalues Some of the replicas might be shorter than the central replica. The problem is thus probably fixed by taking the absolute value of the difference.

This is rather the result of mixing means with quantiles. ~~Incidentally I believe it was fine before and we changed it because people complained, but then we got this bug.~~. I don't think taking the cleanest solution for one we would like the length of the error bar to be yupper-ylower in any case. I think we should just use the median.

Another possible issue might be in validphys.arclength.arc_lengths, namely, in res += integrate.simps(1 + np.square(fdiff), ixgrid[1][1:]) It looks like a square root is missing here: res += integrate.simps(np.sqrt(1 + np.square(fdiff)), ixgrid[1][1:])

Uh. @scarlehoff @RoyStegeman, @enocera could you have a look at this please?

scarlehoff · 2023-02-27T22:23:30Z

This is not branching from master, is it?

It looks like a square root is missing here:
res += integrate.simps(np.sqrt(1 + np.square(fdiff)), ixgrid[1][1:])

Looking at the blame, the square was removed six years ago. Was it intentional?
In any case, I would not change this. The fact that we are not computing an actual arclength is less relevant than keeping the quantity unchanged (as we normally only care about its value wrt other fits/replicas).

RoyStegeman · 2023-02-28T10:30:11Z

Looking at the blame, the square was removed six years ago. Was it intentional?

Yes this is a bit odd... I'd still be in favor of doing the correct thing now though, even if we're mainly just comparing relative lengths anyway. We hardly compare this between different reports, especially older ones, so I don't see huge issues arise if we fix it

scarlehoff · 2023-02-28T10:32:32Z

(Let me tag @scarrazza as well maybe he has some memory of this)

scarlehoff

Please rebase on the current master so that you get the tests running for python 3.9/3.10.

Also, you will need to update also the regression tests since you changed the form of the arclength.

wrt the np.sqrt, as @RoyStegeman says it doesn't really matter since we never use the actual value but I would prefer if some of the people who was around when that change was made could comment just in case it was actually intentional for some obscure reason.

scarlehoff · 2023-03-01T08:43:28Z

+        ylower = np.abs(yvalues - ylower)
+        yupper = np.abs(yupper - yvalues)


Suggested change

ylower = np.abs(yvalues - ylower)

yupper = np.abs(yupper - yvalues)

ylower = np.maximum(yvalues - ylower, 0)

yupper = np.maximum(yupper - yvalues, 0)

I think this is more consistent with what we do in other cases of negative numbers that we don't like (it should not matte much though, it's just a question of taste I guess)

As said above, I think the line should be always of length yupper - ylower. We could even remove the point in the middle and be done with it.

That would also work for me.

As said above, I think the line should be always of length yupper - ylower

This is what the modification already does I think.
@Zaharid, @scarlehoff I pushed the rebased version, but it's not running the tests.

I think it was just that github needs a few minutes to reset (and the test failed because of the regression test since the AL has changed).

This is what the modification already does I think.

Maybe I misunderstood @Zaharid but I think what he would like is just to plot ylower-----yupper instead of yvalues +/- (yupper, ywloer) (and now that I write it down I think I agree, we would like to see the spread, we are already removing outliers at postfit so they are not going (should not) to break the plot)

Except that yvalues should be something like (yupper - ylower)/2. Otherwise we are back to the original problem.

github-actions · 2023-03-01T12:41:28Z

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

Fit Name: NNBOT-60e9c220b-2023-03-01
Fit Report: https://vp.nnpdf.science/rHyJIv7ZQLiTvTWjY0Ntgg==
Fit Data: https://data.nnpdf.science/fits/NNBOT-60e9c220b-2023-03-01.tar.gz

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

Edit by @scarlehoff

arclenghts plots for this report and another similar one
https://vp.nnpdf.science/rHyJIv7ZQLiTvTWjY0Ntgg==/#pdf-arc-lengths
https://vp.nnpdf.science/AdxUWwM8SBCUWe0FPTcEDA==/#pdf-arc-lengths

comane · 2023-03-02T21:19:08Z

-            fmt=".",
+            fmt='',
+            linestyle = '',


Ok I am still not completely sure on whether I understood. However, the following modification should do it.

See https://github.com/NNPDF/nnpdf/pull/1681/files#r1124281268.

scarlehoff · 2023-03-03T09:46:34Z

FAILED tests/test_plots.py::test_plot_xq2

@Zaharid we should just remove the plot tests or change the thresholds because they are a nightmare.

Zaharid · 2023-03-03T10:32:35Z

The plot tests have been fairly stable for years, and seem to have gotten unstable only lately (due to I guess a bigger update in mpl). Would suggest just dealing with it for now by updating the plots. Let's revisit the issue if this keep happening.

Zaharid · 2023-03-07T16:47:58Z

Looks okay to me the way it is now.

github-actions · 2023-03-07T19:12:45Z

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

Fit Name: NNBOT-6b9249a5b-2023-03-07
Fit Report: https://vp.nnpdf.science/L4HDQ463SFK7-nQpiYfrTQ==
Fit Data: https://data.nnpdf.science/fits/NNBOT-6b9249a5b-2023-03-07.tar.gz

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

scarlehoff · 2023-03-07T19:13:47Z

I'm also happy with the new look https://vp.nnpdf.science/L4HDQ463SFK7-nQpiYfrTQ==/#Basespecs0_plot_arc_lengths

Zaharid · 2023-03-08T16:23:05Z

@scarlehoff could you see what the current test failure on master is?

scarlehoff · 2023-03-08T16:28:22Z

Ah, the arclengths changed so the regression of the fit will also see a change of course. And that test is also not run in mac, I forgot about it.

#1681

comane requested review from Zaharid and enocera February 27, 2023 21:34

comane mentioned this pull request Feb 27, 2023

Problem with vp-comparefits #1657

Closed

corrected arclength function.

8cd821d

scarlehoff added the run-fit-bot Starts fit bot from a PR. label Mar 1, 2023

scarlehoff reviewed Mar 1, 2023

View reviewed changes

comane force-pushed the issue_w_vp_comparefits branch from d164a4c to 8cd821d Compare March 1, 2023 19:07

comane added 2 commits March 2, 2023 21:07

changed linestyle of arclength plot

bdd5a57

.csv for tests

cde0050

comane commented Mar 2, 2023

View reviewed changes

modified arclength plot so that error is non negative

b41441c

scarlehoff added run-fit-bot Starts fit bot from a PR. and removed run-fit-bot Starts fit bot from a PR. labels Mar 7, 2023

scarlehoff approved these changes Mar 7, 2023

View reviewed changes

scarlehoff removed the run-fit-bot Starts fit bot from a PR. label Mar 8, 2023

Zaharid approved these changes Mar 8, 2023

View reviewed changes

Zaharid merged commit 1c2feb8 into master Mar 8, 2023

Zaharid deleted the issue_w_vp_comparefits branch March 8, 2023 15:36

scarlehoff added a commit that referenced this pull request Mar 9, 2023

update fit regression test for the change on the arc length computation

7db138f

#1681

		ylower = np.abs(yvalues - ylower)
		yupper = np.abs(yupper - yvalues)

Uh oh!

Conversation

comane commented Feb 27, 2023

Uh oh!

Zaharid commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scarlehoff commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RoyStegeman commented Feb 28, 2023

Uh oh!

scarlehoff commented Feb 28, 2023

Uh oh!

scarlehoff left a comment

Choose a reason for hiding this comment

Uh oh!

scarlehoff Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zaharid Mar 1, 2023

Choose a reason for hiding this comment

Uh oh!

scarlehoff Mar 1, 2023

Choose a reason for hiding this comment

Uh oh!

comane Mar 1, 2023

Choose a reason for hiding this comment

Uh oh!

scarlehoff Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zaharid Mar 3, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Mar 1, 2023 • edited by scarlehoff Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

comane Mar 2, 2023

Choose a reason for hiding this comment

Uh oh!

Zaharid Mar 3, 2023

Choose a reason for hiding this comment

Uh oh!

scarlehoff commented Mar 3, 2023

Uh oh!

Zaharid commented Mar 3, 2023

Uh oh!

Zaharid commented Mar 7, 2023

Uh oh!

github-actions Bot commented Mar 7, 2023

Uh oh!

scarlehoff commented Mar 7, 2023

Uh oh!

Zaharid commented Mar 8, 2023

Uh oh!

scarlehoff commented Mar 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Zaharid commented Feb 27, 2023 •

edited

Loading

scarlehoff commented Feb 27, 2023 •

edited

Loading

scarlehoff Mar 1, 2023 •

edited

Loading

scarlehoff Mar 2, 2023 •

edited

Loading

github-actions Bot commented Mar 1, 2023 •

edited by scarlehoff

Loading