patches to ldmx-sw following compiler update #1640
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- copy over refactor from LDMX-Softwar/fire and merge in our improvements - build into submodule - still need to test that it works after refactor and update to the newer Python C API
|
Can you add to the TODO list? |
all tests compiling, Framework test passing the run
remove deprecated functions and use PyConfig style configuration before the initialization of the python interpreter
- only open config file once - close python interpreter if an exception is thrown after it is opened
we do not want python to parse the argv for us, we are already parsing it and so when python parses it, the first argument is dropped as if it was the 'python' command calling a script
this is only because we erroneously have a list of parents for sim particles when all sim particles only ever have exactly one parent
we have been well established for only using the container for building ldmx-sw for several years now. Additionally, this check is difficult to maintain because there isn't a standardized way to check whether you are in a container. The CI and I get this warning when using podman because podman does not create either of the files listed in this check.
it is not used anywhere in the tests
- don't try to hold a reference, just use safe_extract in place when needed or copy - define an empty object for logRules so that it is alive long enough to be used
older compilers didn't catch this because the code _does_ increment nPos within the loop over adc values, but it never accesses its value for something else
check for overlay event below when calling `nextEvent` on the overlay
|
I can't figure out the Hcal testing right now (I was hoping the additional warnings would point me towards something but they did not) and I want to wait on the nullptr dereferencing related to other developments, so I am marking this as Ready for Review. The changes are all "small" in the sense that they are not expected to affect the physics. The previous PR Validation run did not show any differences and I am expecting the same again. |
tomeichlersmith
commented
Mar 4, 2025
Comment on lines
+39
to
+40
| Parameters run(const std::string& root_object, const std::string& pythonScript, | ||
| char* args[], int nargs); |
There was a problem hiding this comment.
I'm sneaking in a refactor of the Configure/Python ecosystem that does a few things:
- Removes the extraneous
ConfigurePythonclass in favor of a function that runs the python script and returns aParametersof the configuration extracted. - Updates
Parametersto have shorteraddandgetmethods. - Promotes the "root object" that is used to extract the configuration to the argument so that hypothetically other programs could be written that use the same configuration system but with a different python object (e.g. https://github.com/LDMX-Software/ldmx-tb-online/tree/main/reformat could be brought in as a submodule with its own executable if we desired).
| if (fabs(track.getCentroid() - nextTrack.getCentroid() < | ||
| 3 * maxDelta_)) { | ||
| if (fabs(track.getCentroid() - nextTrack.getCentroid()) < | ||
| 3 * maxDelta_) { |
There was a problem hiding this comment.
@bryngemark I think this is just a misplaced closing parentheses since the newer compiler warned me that fabs(bool) doesn't make any sense.
There was a problem hiding this comment.
i agree with this change
tvami
reviewed
Mar 4, 2025
tvami
approved these changes
Mar 5, 2025
bryngemark
approved these changes
Mar 5, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I am updating ldmx-sw, here are the details.
What are the issues that this addresses?
This is following up on LDMX-Software/dev-build-context#103 . I will maintain an checklist here of patches that I need to make before I feel comfortable merging.
configure-quick
The
configure-quickbuild does not enable as many warnings, but I still get the big ones so its a helpful place to start. After I patch these up, I can move to a configuration that enables more warnings (e.g. our standardconfigure).std::iteratoris deprecated,need to find replacement forno replacement, just remove it and the callers deduce if the iterator class has the required functionsHgcrocDigiCollectionrecon::PFTruthProducerwhen usingldmx::SimParticle::getParentsHcalDigiPipelineTestis now failing 812 / 1000 of the test energies. All of the test hits being reconstructed return adaq_peof 0 rather than something close to the real value. The tests below 40PE are passing because 0 is within 40 of the true value. I don't think this is as apocalyptic as it seems because the PR validations all pass and those include distributions on non-zero PE hits in the HCal. I am guessing it has to do with how the test is creating the hits or how Catch2 is working with the new compiler.ldmx-sw/DetDescr/src/DetDescr/DetectorIDBindings.cxx
Lines 2 to 3 in 169ee85
configure
The regular
configurebuild enables additional warnings and clang tidy.std::moveinProcess::getDummy- compiler can use copy elision to place the construction where it is first usedRunHeaderwhich is inlined fromimportRunHeaderschannelandmsg_levelin Logger{}as default forlogRulesmatchcodeNtuplemanagerTest.cxx:124-132Catch::Matchers::UnorderedEquals, not sure if I'm misusing it thoughnPosset but not used inEventReadoutProducer.cxx:64TrigScintTrackProducer.cxx:333overlayEvent_will never be NULLKaonphysics::DumpDecayDetails(lots of them)particleIndexset but not used inLHEReaderandLHEPrimaryGeneratorG4Stringcopy constructorGetLogicalVolumein a lot of the user actionsTaggerHitFiltersame memory used after free asEcalSDRecon/DBScanClusterBuilder.cxx:132, would be null dereference if size is zero