-
Notifications
You must be signed in to change notification settings - Fork 21
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
Sample scripts for validating future samples #1216
Conversation
hi @tylerhoroho-UVA , thanks for this! did you try using the validation module for plotting at all? |
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 concur with @bryngemark , this is great and thank you for sharing :)
My comments are 100% cleaning comments.
- Remove the undefined functions (I added some suggestions).
- Format the code according to our style guide so its easier for other folks to collaborate.
- Make the hard brem energy configurable? I'm not sure if this is desirable since we often set-it and forget-it, but its something to think about.
Formatting
In container images v4 and newer, clang-format
is already installed so you can.
ldmx clang-format -i --style=Google DQM/include/DQM/SampleValidation.h DQM/src/DQM/SampleValidation.cxx
# commit these changes
Hi @bryngemark, I made some effort to use the Validation module, but integration with this module would be more difficult than keeping everything contained in DQM. Since Einar developed the Validation module, maybe I could work with them to integrate the python code into Validation if that's where it should be kept? |
Right, so what I think we ideally want is a combination of DQM for doing all the ldmx-sw processing/analyzing (like histogram booking and filling) and then a mirroring validation script that is just used to find those histograms, draw them, compare them, etc etc. So keeping what you have right now but just adding the Validation bit. If you want inspiration you could look at Tom's recent PR here: #1215 |
I can put in some more effort to integrating Validation into the workflow before deciding it's too much work. Thanks for linking Tom's PR; I think it will be helpful! |
Co-authored-by: Tom Eichlersmith <31970302+tomeichlersmith@users.noreply.github.com>
Co-authored-by: Tom Eichlersmith <31970302+tomeichlersmith@users.noreply.github.com>
c5d06e8
to
909d2b2
Compare
DQM/src/DQM/SampleValidation.cxx
Outdated
@@ -117,6 +121,8 @@ namespace dqm { | |||
if (pdgid > 2300) label = 16; //exotic (e.g., baryon with strangeness) |
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.
Have you considered what should happen if some PDGID doesn't match any of your defined criteria?
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 have considered this and plan to make changes to account for this. The current setup was put together quickly so that Jessica could proceed with validation plots. I think I will fork "exotic" into "strange baryons" and "something else" (which would then require looking at the event level to see the mystery particle). I can also expand the A' label to be any dark mediator.
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.
Sounds great. Just a catch-all in the end for anything unexpected can turn out to be very useful.
density=True) | ||
|
||
d.plot1d("SampleValidation/SampleValidation_pdgid_harddaughters" "PDG ID of hard primary daughter", | ||
tick_labels=pdgid_labels, |
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 is really great. Makes it SO much easier to read and grasp the plot!
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.
+1
DQM/src/DQM/SampleValidation.cxx
Outdated
@@ -89,7 +87,7 @@ namespace dqm { | |||
} | |||
|
|||
int SampleValidation::pdgid_label(const int pdgid) { | |||
int label = 0; | |||
int label = 18; |
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.
Cool! Just add a comment that this is a catch-all/overflow bin for code readability
DQM/src/DQM/SampleValidation.cxx
Outdated
|
||
if (pdgid == 3122 || pdgid == 3222 || pdgid == 3212 || pdgid == 3112 || pdgid == 3322 || pdgid == 3312) label = 16; // strange baryon | ||
|
||
if (pdgid > 10000) label = 15; //nuclei |
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.
Is it worth differentiating between general nuclei and light ions here? The production of alpha, deuterium, and tritium from PN interactions is non-trivial.
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 can include this distinction since I meant for the label to mainly catch remnants of a tungsten explosion. Do you think "light"/"heavy" nuclei is granular enough, with "light" being only hydrogen or helium nuclides?
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.
The bertini cascade will attempt to take nucleons being ejected from the cascade that are "similar" and bunch them together into deuterium, tritium, alpha (called "Coalescence") so you can get these even for quite ordinary interactions.
There is a version of a test for what counts as a light ion here (that probably should live somewhere else...)
https://github.com/LDMX-Software/ldmx-sw/blob/8e6e9e195eb71ad78f7aa94648a179342a12e1f6/Biasing/include/Biasing/PhotoNuclearTopologyFilters.h#L62C1-L78
double hard_thresh = 2500; | ||
|
||
//Loop over all SimParticles | ||
for (auto const& it : particle_map) { |
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.
Not something that needs to be fixed but something that is useful is that when iterating over a map you can do
for (auto const& [id, particle] : particle_map) { ... }
To define the id and particle variables directly without having to do it.second :)
DQM/src/DQM/SampleValidation.cxx
Outdated
|
||
std::vector<int> primary_daughters; | ||
|
||
double hard_thresh = 2500; |
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.
Why are we defining hard_thresh down here and redefining it later?
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 needed to declare hard_thresh outside the first for loop since it is used in later loops. But in principle I should only declare it here without assigning it a value yet. This was likely a remnant from when hard_thresh was a hard-coded value. I can remove the value definition but will still keep the variable declared here.
density=True) | ||
|
||
d.plot1d("SampleValidation/SampleValidation_pdgid_harddaughters" "PDG ID of hard primary daughter", | ||
tick_labels=pdgid_labels, |
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.
+1
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.
All good, assuming all of the changed files have been formatted before merged (they might already be)
@tomeichlersmith are the failed build tests a consequence of needing to rebase, or something else? if all is good then i think we can merge this |
The failed tests are due to UMN losing its network connection on Monday and thus the tests were unable to download an example CSV table condition.
This is the third time this has happened... we may want to move our URL test somewhere else (or stop testing it since we don't use the internet retrieval of conditions right now). |
I'm not super familiar with our test setup, but is there a way to mark that test as not fatal of it fails? |
Is the plan to include this in some of the CI DQM configs btw? |
Dear all,
I think a great solution (if possible) would be to put copies of the
document in two locations (e.g. UMN and SLAC or Lund) and then fail only
if /both/ fail.
Jeremy
…On 11/29/23 14:02, Einar Elén wrote:
I'm not super familiar with our test setup, but is there a way to mark
that test as not fatal of it fails?
—
Reply to this email directly, view it on GitHub
<#1216 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFUPYHBWUHHLGQNN5OLCELYG6IFVAVCNFSM6AAAAAA5WCOQ62VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZSGYYTSMRZGU>.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
i think @EinarElen and @jmmans both make some excellent points here. we could punt on (as in go ahead and merge now) copying stuff into the CI DQM, maybe think a little more about what we'd like to include. copying the conditions to both slac and Lund could work. |
I am updating ldmx-sw with sample validation scripts discussed in the 9/25/23 SWAN meeting. Here are the details.
Check List
I successfully compiled ldmx-sw with my developments
I ran my developments and the plots shown in 9/25/23 SWAN meeting show that they are successful.
I have included a sample configuration file for running the code (in .txt form that needs converted to .py, since GitHub doesn't support .py attachments to PRs):
val_config.txt
It can be executed with the syntax
ldmx fire val_config.py <input file to validate> <output file name, with .root extension>