Add a summary SpectrumConfig object to generator outputs#1845
Add a summary SpectrumConfig object to generator outputs#1845michaelmackenzie wants to merge 7 commits into
Conversation
|
Hi @michaelmackenzie,
which require these tests: build. @Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main. ⌛ The following tests have been triggered for 4a17888: build (Build queue - API unavailable) |
|
☔ The build tests failed for 4a17888.
N.B. These results were obtained from a build of this Pull Request at 4a17888 after being merged into the base branch at 3d6f2fe. For more information, please check the job page here. |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 8e2fd2d: build (Build queue - API unavailable) |
|
☔ The build tests failed for 8e2fd2d.
N.B. These results were obtained from a build of this Pull Request at 8e2fd2d after being merged into the base branch at 3d6f2fe. For more information, please check the job page here. |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 1ba6baf: build (Build queue - API unavailable) |
|
☔ The build tests failed for 1ba6baf.
N.B. These results were obtained from a build of this Pull Request at 1ba6baf after being merged into the base branch at 83b5e2f. For more information, please check the job page here. |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 00a3573: build (Build queue - API unavailable) |
|
☀️ The build tests passed at 00a3573.
N.B. These results were obtained from a build of this Pull Request at 00a3573 after being merged into the base branch at 83b5e2f. For more information, please check the job page here. |
brownd1978
left a comment
brownd1978
left a comment
There was a problem hiding this comment.
Thanks for introducing this Michael, it's an important addition to our simulation bookkeeping.
As defined, SpectrumConfig is a passive record of what the generators actually do. A stronger design is to also use the object to make selections in the generator, tying together implementation and recording. I believe this could be done reasonably efficiently using a map to associate values with variables, perhaps even inside ParticleGeneratorTool, which would simplify and standardize the individual generator implementation.
If the PDF associated with each restricted variable is known you could actually compute the fraction in SpectrumConfig. Please consider adding a constructor taking the PDF as a function as input for that case, to again standardize and simplify downstream code, either here or as a future upgrade.
The current design depends on the the input variables being independent, that should be clearly stated. I can think of counter examples. It might be possible to generalize SpectrumConfig to handle that, but it's not needed for this first implementation.
'type' makes more sense as an attribute of the variable, not the spectrum config. Then you can get rid of the 'other' type value, which is currently used to describe mixed flat and non-flat variable. If you provide the PDF function you could also have a value to describe that.
| xmax_(xmax) {} | ||
|
|
||
| std::string name_ ; | ||
| double fraction_ ; |
There was a problem hiding this comment.
I suggest adding comments to describe these variables
| if(verbose) printf(" Total fraction: %.4e\n", fraction); | ||
| return fraction; | ||
| } | ||
|
|
There was a problem hiding this comment.
I suggest adding a function to test a collection of values against the limits and return true/false if the values are in range, to be used downstream in a filter or other mechanism. To be proof against ordering errors I suggest to provide these as a map, with values keyed to the variable name, with the function requiring the names all match.
| class SpectrumConfig { | ||
| public: | ||
| // Indicate the broad class of primary simulations this falls under | ||
| enum Type {kPhysical = 0, kFlat, kOther}; |
There was a problem hiding this comment.
Why is this enum associated with the composite object and not the individual variable? I could imagine a config restricting the range of flat and non-flat variables (say phi and theta) in a single object.
|
|
||
| public: // allow direct access/manipulation of the fields | ||
| std::vector<RestrictedVar> vars_; | ||
| int type_; |
There was a problem hiding this comment.
Why isn't this data member strongly typed?
3 participants
The goal of this data product is to simplify downstream workflows that need to know information about a dataset's primary generator configuration. This includes samples with restricted energy and cos(theta_z) regions. I've added default output of this product to all generators, even where the information is trivial, so downstream workflows will always find it and it can be updated to add more information if restrictions change.