Correct phi0 due to expected output range from HTF

[cbravo135]

Fix bug that slipped through our checks.

[pbutti]

Fixes the phi0 bug reported on recon-tracking slack channel.
Reverts one of my buggy changes.

[cbravo135]

FYI, this PR will need the reference plots for 2015 and 2016 integration tests changed when it is merged. I already prepared them and placed them in the appropriate directories at JLab with the label 20210428.

[normangraf]

I'm confused as to why any of the reference plots have to be changed. I thought it was claimed that the integration tests did not catch this bug. If that's true, then the plots didn't change when the bug was introduced, so why are they changing when the bug is fixed? If the bug did change the plots (as it seems now they did), then it appears due diligence was not paid when the tests failed last time around and the reference plots were simply updated.

[pbutti]

Hi,
I thought a bit about this in detail and i think that the first thing to do would be check the integration tests plots. I have written a small basic tool for comparison and it's in my personal github.
https://github.com/pbutti/hps-java-validation

Maybe we should check these plots before updating the reference.

And to help clarify the claim "the tests didn't catch this", maybe would be more fair to say that I, personally, didn't catch this issue when running tests. When we merged the tracking PR some of the integration tests plots were checked in detail, but probably i didn't check the phi0 distribution (so another error from my side). So we checked that some of the FEE / V0 distributions were close enough (within stats errors/ and we proceeded to update them. Again, would make sense to recheck these plots (a plotting validation should become good practice when integration tests fail from now on, i think) by plotting before/after this change.

[normangraf]

@pbutti I concur. We should check the before/after plots using your tool and use that to document any changes to the reference plots.

[cbravo135]

The phi0 distributions are not one of the variables included in the integration test plots.

[cbravo135]

I can clarify a bit what happened. We did expect the reference plots to change with the changes to the math libraries as you expect small (negligible) differences using a new method of calculating trig, etc. functions. It is simply untrue that the reference plots were not carefully checked when they failed before this code was merged: https://indico.slac.stanford.edu/event/6753/contributions/1685/attachments/763/2107/validation_latex.pdf @normangraf was also in attendance during this meeting where we discussed at length the change to the reference plots and everyone agreed then that we should move forward with updating them during that meeting. Due to the nature of this bug, it really only essentially numerically puts phi0 in a different range than expected (that which is used by the karimaki fit is not the same that we end up plotting). The change in the plots the first time around was NOT only due to this bug, as @normangraf appears to be assuming. The plots are still not back to the same as they were before merging the code, as is expected. The integration test plots do not include a track phi0 plot, this is why this bug slipped through. I am pretty happy we managed to catch (and fix) it before doing any serious production.

[cbravo135]

IMO, going through the whole exercise of carefully checking them again is more work than it is worth, since we only reverted code to what it was before merging any of this code, and I am already doing a more fully encompassing check by running on the sample partitions (how we noticed the bug in the first place).

[bloodyyugo]

Does this really need to be changed? If so, you should get rid of the comment, but the comment is there because I wasn't sure. If this method isn't used, probably just get rid of it.

return FastMath.atan2(y, x);// mg 9/20/17...I think this is the wrong order...should be atan2(y,x)

[cbravo135]

@bloodyyugo I checked this by adding a print statement and running a few steering files to see if I see the warning print. I do think there are likely a bunch of unused methods sprinkled throughout the project, but the best way to check if this one in particular is really unused is to just completely remove it and try to compile. I will look into this.

[cbravo135]

As a reference, here is the set of plots checked for PhysRun2016MollerReconTest (note the absence of track phi0)

Histogram			Ref Entries	Test Entries	Ref Mean	Test Mean	Ref RMS		Test RMS

Moller Bottom Track Momentum    2126		2124		1.15715615	1.15714438	0.10987261	0.10987513
Moller Invariant Mass           2126		2124		0.04962023	0.04962698	0.00226308	0.00225631
Moller Momentum                 2126		2124		2.25821430	2.25822934	0.05051941	0.05036369
Moller Top Track Momentum       2126		2124		1.10471650	1.10470503	0.11076831	0.11063843
Moller Track Chisq Prob         4252		4248		0.72111365	0.72091224	0.29310489	0.29314919
Moller Track Chisq per DoF      4252		4248		1.90477206	1.90414008	1.40693124	1.40689448
Moller Track Momentum           4252		4248		1.13093633	1.13092470	0.11339437	0.11333214
Moller Track Number of Hits     4252		4248		5.68132643	5.68079096	0.46596215	0.46617017
Moller Vertex Chisq             2126		2124		7.90860612	7.90326583	5.81830124	5.81757883
Moller Vertex x                 2126		2124		0.06837632	0.06853435	0.16096100	0.16080915
Moller Vertex y                 2126		2124		-0.06167703	-0.06164592	0.02300046	0.02296410
Moller Vertex z                 2126		2124		2.64604566	2.65501748	3.76051806	3.75258093
Moller x Momentum               2126		2124		0.00136262	0.00135689	0.00244536	0.00244321
Moller y Momentum               2126		2124		0.00067672	0.00067438	0.00178887	0.00178730
Moller z Momentum               2126		2124		2.25821173	2.25822679	0.05052010	0.05036438
Moller Bottom Track Momentum    1895		1892		1.15413803	1.15399402	0.10952022	0.10953969
Moller Invariant Mass           1895		1892		0.04671788	0.04672200	0.00202774	0.00202502
Moller Momentum                 1895		1892		2.25684406	2.25696130	0.04991803	0.04981587
Moller Top Track Momentum       1895		1892		1.10618037	1.10640318	0.11001070	0.10993282
Moller Track Chisq Prob         3790		3784		0.71820162	0.71827564	0.29445847	0.29442535
Moller Track Chisq per DoF      3790		3784		1.89724731	1.89719346	1.41093213	1.41079508
Moller Track Momentum           3790		3784		1.13015920	1.13019860	0.11235435	0.11228671
Moller Track Number of Hits     3790		3784		5.66886544	5.66860465	0.47062136	0.47071485
Moller Vertex Chisq             1895		1892		13.98894895	13.97618373	6.80297080	6.79425397
Moller Vertex x                 1895		1892		-0.16323463	-0.16323762	0.09729474	0.09727297
Moller Vertex y                 1895		1892		-0.06909497	-0.06911108	0.01920820	0.01920198
Moller Vertex z                 1895		1892		-4.29999911	-4.29999911	0.00000046	0.00000045
Moller x Momentum               1895		1892		0.00098650	0.00098353	0.00204136	0.00203930
Moller y Momentum               1895		1892		0.00060873	0.00060981	0.00182766	0.00182881
Moller z Momentum               1895		1892		2.25684209	2.25695932	0.04991861	0.04981645
Moller Bottom Track Momentum    2131		2130		1.15912199	1.15925089	0.11075950	0.11064092
Moller Invariant Mass           2131		2130		0.04975644	0.04975788	0.00233318	0.00233199
Moller Momentum                 2131		2130		2.26126494	2.26131476	0.05439717	0.05434285
Moller Top Track Momentum       2131		2130		1.10333246	1.10324054	0.11120179	0.11118805
Moller Track Chisq Prob         4262		4260		0.72062844	0.72059187	0.29335553	0.29322770
Moller Track Chisq per DoF      4262		4260		1.90201658	1.90155132	1.40587201	1.40544110
Moller Track Momentum           4262		4260		1.13122723	1.13124571	0.11443282	0.11439575
Moller Track Number of Hits     4262		4260		5.68559362	5.68544601	0.46427902	0.46433800
Moller Vertex Chisq             2131		2130		3.21955008	3.22106682	3.97051015	3.98187977
Moller Vertex x                 2131		2130		0.11198988	0.11195612	0.32590914	0.32565444
Moller Vertex y                 2131		2130		0.00500734	0.00507285	0.10983889	0.10989058
Moller Vertex z                 2130		2129		3.11404182	3.11469940	4.02566549	4.02659159
Moller x Momentum               2131		2130		0.00092131	0.00091776	0.00362886	0.00362430
Moller y Momentum               2131		2130		-0.00055411	-0.00055643	0.00232012	0.00231945
Moller z Momentum               2131		2130		2.26126056	2.26131039	0.05439795	0.05434361

[cbravo135]

It does really need to change: https://commons.apache.org/proper/commons-math/javadocs/api-3.3/org/apache/commons/math3/util/FastMath.html#atan2(double,%20double)
This is used in this file, checking to see if it can really be removed safely would take some real work. I removed the comment.

Does this really need to be changed? If so, you should get rid of the comment, but the comment is there because I wasn't sure. If this method isn't used, probably just get rid of it.

return FastMath.atan2(y, x);// mg 9/20/17...I think this is the wrong order...should be atan2(y,x)

[pbutti]

Hi! What's the plan with this PR?
I checked @cbravo135 dump and saw that the mean values are basically not moving.
@normangraf are you happy with this check by Cam or do you want to see the actual plot histograms? I think I'm happy with the mean dump he provided and approving the PR.
(Also I was the person that introduced the bug, so I understand the way it was removed.)

[normangraf]

@pbutti I've been too busy with other tracking issues to look into this. I was expecting @bloodyyugo or @omar-moreno to approve this once they are convinced.