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

Avoid frtree to be written in plots.root + other updates for standalone workflow (samples and only initialStep for BH, STD and CE builds) #328

Merged
merged 13 commits into from
Jul 6, 2021

Conversation

mmasciov
Copy link
Collaborator

@mmasciov mmasciov commented Jul 1, 2021

Brings size of plots.root down by ~300x.
Results are identical, e.g., can compare:
(1a) http://uaf-10.t2.ucsd.edu/~mmasciov/MkFit_pixelPairStep/simval_test_iter4/
(2a) http://uaf-10.t2.ucsd.edu/~mmasciov/MkFit_pixelPairStep/SAV_pixelPairStep/SAV_pixelPairStep_iter4/

or

(1b) http://uaf-10.t2.ucsd.edu/~mmasciov/MkFit_pixelPairStep/simval_test_iter22/
(2b) http://uaf-10.t2.ucsd.edu/~mmasciov/MkFit_pixelPairStep/SAV_pixelPairStep/SAV_pixelPairStep_iter22/

As for BH, STD and CE builds (not multi-iter), results are consistent after using only initialStep.
E.g., http://uaf-10.t2.ucsd.edu/~mmasciov/MkFit_pixelPairStep/SVA_noMultiIter_testV3/ to be compared to http://uaf-10.t2.ucsd.edu/~mmasciov/MkFit_pixelPairStep/SVA_multiIter/simvalseed_test_iter4/

Note (1): samples are latest available, including fix in PR #330.
Note (2): for non-multi-iter builds (BH, STD and CE), this limits validation to initialStep. Potential extension of BH and STD to multi-iter is not addressed in this PR.
Note (3): few of the scripts (not of common use anymore) still refer to SNB architecture, or are labeled as "SNB". As phi1 (SNB) is still online, this has not been changed in this PR.

@mmasciov mmasciov force-pushed the PlotValidation_fix branch from 808160a to 67b4e17 Compare July 1, 2021 17:23
@mmasciov
Copy link
Collaborator Author

mmasciov commented Jul 1, 2021

The PR is ready for review/merge on my side.
The first commit fixes the issue with unnecessary tree writing.
The second commit updates all xeon_scripts to the latest samples.
The third commit updates all val_scripts to the latest samples.
The fourth commit updates web scripts to collect either standard benchmarks, or multi-iter ones.

@mmasciov mmasciov force-pushed the PlotValidation_fix branch from 67b4e17 to b39bbbc Compare July 1, 2021 17:27
@mmasciov mmasciov changed the title Avoid frtree to be written in plots.root Avoid frtree to be written in plots.root + other updates for standalone workflow (samples and only initialStep for BH, STD and CE builds) Jul 3, 2021
@slava77
Copy link
Collaborator

slava77 commented Jul 3, 2021

I noticed that validation-cmssw-benchmarks-multiiter.sh works only with forConf.
Does this PR allow to expand to forPR? or is the idea to reduce STD and BH to be only in the initialStep?

@mmasciov
Copy link
Collaborator Author

mmasciov commented Jul 3, 2021

This only reduces STD and BH, as well as the original CE, to initialStep, while we use MultiIter (an extension of CE) for multiple iterations.
I would go with this first. Then, we can think of extending STD and BH to other iterations, but so far this has never been tested, and would require non-negligible testing/changes.
Also, I'm personally not sure it makes sense to keep using STD and BH if we are not going to use any of the two.

@mmasciov
Copy link
Collaborator Author

mmasciov commented Jul 4, 2021

This PR is ready to be reviewed/merged. No additional change appears to be required in order to limit validation of builds other than multi-iter to initialStep-only, which is the goal of this PR, together with updating to latest samples and reduce size of validation output by avoiding un-necessary copies of trees.

Copy link
Collaborator

@slava77 slava77 left a comment

Choose a reason for hiding this comment

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

the files are now on phi3

@mmasciov
Copy link
Collaborator Author

mmasciov commented Jul 4, 2021

All samples have been replaced with the latest by @slava77 (with fix in PR #330).
PR should be now ready for merge (conversations have been resolved, and original comment has been expanded).

Copy link
Contributor

@leonardogiannini leonardogiannini left a comment

Choose a reason for hiding this comment

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

I don't understand this part https://github.com/trackreco/mkFit/pull/328/files/9d9a1beab800d77565f308f2f05e9c7b5008d1a7#diff-9bd117b4474e1261389e0f33d9f898dc985c780ab0e7c9c12339ce68867a0e66R125, which is already in MkBuilder. I also seemed to have problems with selection before relabeling here https://github.com/trackreco/mkFit/blob/devel/mkFit/MkBuilder.cc#L936-L937 if the order of our iterations is not the same as cmssw.

@mmasciov
Copy link
Collaborator Author

mmasciov commented Jul 6, 2021

I don't understand this part https://github.com/trackreco/mkFit/pull/328/files/9d9a1beab800d77565f308f2f05e9c7b5008d1a7#diff-9bd117b4474e1261389e0f33d9f898dc985c780ab0e7c9c12339ce68867a0e66R125, which is already in MkBuilder. I also seemed to have problems with selection before relabeling here https://github.com/trackreco/mkFit/blob/devel/mkFit/MkBuilder.cc#L936-L937 if the order of our iterations is not the same as cmssw.

That part was aimed at reducing the size of the CMSSW valtree.
However, as you noted, the same could be achieved by requiring "--num-iters-cmssw 1" in the validation script, for CMSSW.
Just for confirmation, results are unchanged while the size of the CMSSW valtree is reduced by a factor of ~6x:

Hence, I reverted my change in the last commit, and just added "--num-iters-cmssw 1" in the non-multi-iter CMSSW validation script, for the CMSSW validation command.

@slava77 slava77 merged commit 8acd939 into devel Jul 6, 2021
@osschar osschar deleted the PlotValidation_fix branch July 12, 2021 22:31
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.

4 participants