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
[HGCal] Modify MultiTrackValidator plotting for easier browsing of HGCal plots #26100
Conversation
If Validation plots are produced using the 'separate' option, a static web page will be created with the proper links to all the images. This commit will add in that specific directory the index.php file, and all its needed components, to be able to browse the content of the directory automatically, hence visualizing all images at a glance.
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26100/8672
|
A new Pull Request was created by @apsallid for master. It involves the following packages: Validation/RecoTrack @andrius-k, @kmaeshima, @schneiml, @cmsbuild, @jfernan2, @fioriNTU can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
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.
Could you show an example of how the output directory looks like (in the web)?
@@ -1964,7 +1964,7 @@ def _doStats(h, col, dy): | |||
st.SetX1NDC(startingX) | |||
st.SetX2NDC(startingX+0.3) | |||
st.SetY1NDC(startingY+dy) | |||
st.SetY2NDC(startingY+dy+0.15) | |||
st.SetY2NDC(startingY+dy+0.25) |
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.
This change increases the height of the stat box, right? IIRC the space between stat boxes of different histograms/graphs was already rather small. Can you show that this change does not lead the stat boxes to overlap?
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.
From your example I see that your case has indeed more lines in the stat box than MTV has (by default). I'm not against that, but then the height should be made e.g. configurable or automated, and the height should propagate to the "line height" (magical dy -= 0.19
below) as well.
@@ -2532,6 +2532,7 @@ def create(self, tdirectoryNEvents, requireAllHistograms=False): | |||
def draw(self, *args, **kwargs): | |||
kargs = copy.copy(kwargs) | |||
kargs["ratio"] = False | |||
kargs["separate"] = False |
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.
Could you explain why this change is needed?
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.
Ciao Matti,
the separate option was giving conflicts (crashes, IIRC) if coupled with the PlotOnSideGroup, which sort of makes sense since the 2 concepts are somehow "opposite". That's why I defaulted that option to False in this specific case. I guess you never tested the 2 coupled together, right?
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.
It's indeed very likely that I did not test these two together, and on a further thought I do agree that the "separate" concept doesn't really apply to the PlotOnSideGroup. Thanks for the explanation!
@@ -1293,5 +1293,14 @@ def _doPlots(self, plotterFolder, dqmSubFolder): | |||
print("Typically this is a naming problem in the plotter configuration") | |||
sys.exit(1) | |||
|
|||
if (self._plotterDrawArgs.get("separate", False) == True): |
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.
I'd rather go with
if (self._plotterDrawArgs.get("separate", False) == True): | |
if self._plotterDrawArgs.get("separate", False): |
downloadables = ["index.php", "res/jquery-ui.js", "res/jquery.js", "res/style.css", "res/style.js", "res/theme.css"] | ||
for d in downloadables: | ||
if not os.path.exists("%s/%s" % (newdir,d)): | ||
import urllib |
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.
Could the import be moved to the top of the file?
Thanks a lot for the super fast reply! This is how the output looks like in the web: http://apsallid.web.cern.ch/apsallid/HGCalValidationPlots/ And if you click on any of these, you will see at the end a Browse Folder button. |
The code-checks are being triggered in jenkins. |
Looks good to me, thanks. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26100/8673
|
Pull request #26100 was updated. @andrius-k, @kmaeshima, @schneiml, @cmsbuild, @jfernan2, @fioriNTU can you please check and sign again. |
@cmsbuild, please test (doesn't really test anything but anyway) |
The tests are being triggered in jenkins. |
please test |
The tests are being triggered in jenkins. |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
+1 |
1 similar comment
+1 |
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
For the plotting part of the HGCalValidator package we take advantage of the tools already created by the MultiTrackValidator.
However, one option was added that when set to true it will produce a webpage with all plots to be able to
visualize all images at a glance.
PR validation:
This PR depends on PR #26099. So, after the harvesting step, you can run the makeHGCalValidationPlots.py script with the new separate option e.g:
@felicepantaleo @rovere @cseez @malgeri
@makortel Could you please comment?