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

Move validation configs to 8 GeV, intro reduced LDMX validation, add tracking to ecal pn #1385

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

tvami
Copy link
Member

@tvami tvami commented Aug 12, 2024

I am updating ldmx-sw, here are the details.

What are the issues that this addresses?

Resolves #1384

  • Moved ecal pn to 8 GeV
  • Given that the tracking is in reasonable shape, I included in the validation suit
  • Added a new validation config for the reduced geometry (which is still at 4 GeV)

Check List

  • I successfully compiled ldmx-sw with my developments
  • I ran my developments and the following shows that they are successful.

This needs #1383 to be merged first. After that's done, I'll rebase

@tvami tvami changed the title Iss1384 validation update Move validation configs to 8 GeV, intro reduced LDMX validation, add tracking to ecal pn Aug 12, 2024
@tvami tvami force-pushed the iss1384-validation-update branch from 5571c59 to fd1f2fa Compare August 12, 2024 18:51
@tvami tvami force-pushed the iss1384-validation-update branch from fd1f2fa to df25e7c Compare August 12, 2024 18:56
@tvami
Copy link
Member Author

tvami commented Aug 12, 2024

@tomeichlersmith is there anything I can do to generate gold for the reduced ldmx?

This needs #1383 to be merged first. After that's done, I'll rebase

I realized this can be merged since the gold doesnt exists.

@tomeichlersmith
Copy link
Member

The current design (which I'm not commited to but it is functional) is to have files follow specific naming conventions and then the CI detects what validation samples exist based off those assumptions.

https://github.com/LDMX-Software/ldmx-sw/tree/trunk/.github/actions/validate#assumptions

Both the gold-generation and the pr-validation use this action to run the samples so that they are run in the same way and they get which samples to include in generation/validation by simply using all directories within the .github/validation_samples directory.

https://github.com/LDMX-Software/ldmx-sw/tree/trunk/.github/actions/generate-matrix

So, to add a new sample to be included in validation, the general procedure would be

  1. Develop a config script that creates the sample you want and fills the histograms you want
  2. Put this script in .github/validation_samples/my-sample/config.py
  3. Update config.py to take the run number and number of events from the environment and have the output histogram file be named hist.root. (for example)
  4. Run config.py locally to create a first copy of hist.root and gen.log.
  5. Rename the created ROOT and log files to be gold.root and gold.log
  6. Commit those gold files

This process is arduous but I couldn't think of anything better at the time. We can probably improve this but I'm not sure how many more validation samples we will want to include. I believe ee will quickly approach the computing limit of our free-tier GitHub workflows which is undesirable.

@tvami
Copy link
Member Author

tvami commented Aug 12, 2024

Thanks Tom! I only miss steps 4-6 then, I'll commit those soon

@tvami
Copy link
Member Author

tvami commented Aug 13, 2024

I'll commit those soon

done now! also about the new version, maybe let's do it after I'm done with iss1387 (which I could do tomorrow [Tue])

Copy link
Member

@tomeichlersmith tomeichlersmith left a comment

Choose a reason for hiding this comment

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

Looks good to me :)

I'm happy to add more tests and let them tell us where things are wonky.

@tvami
Copy link
Member Author

tvami commented Aug 13, 2024

@tomeichlersmith when I run the CI here
https://github.com/LDMX-Software/ldmx-sw/actions/runs/10372637362/job/28715827843#step:4:9
it says

error: pathspec '.github/validation_samples/reduced_ldmx/gold.root' did not match any file(s) known to git

I'm a little bit confused since that file is added

@tomeichlersmith
Copy link
Member

tomeichlersmith commented Aug 13, 2024

Ah yes I forgot, the golden files are retrieved from a checkout of trunk, so attempting to get a sample that is not on trunk yet will fail.

A few paths forward

  • Leave the CI as is. We merge without the tests passing because this acknowledges we are introducing new tests via a new sample.
  • Update CI to skip samples that weren't found. I think this is preferable, we could issue a warning in the CI as well, but this development would not be simple. (Both the step that failed using git checkout and the generate-matrix action would need to be updated to allow for a mismatch between the trunk and dev-branch sample sets.) Again, similar to what I've said elsewhere, not sure if this development is worthwhile since I'm not sure how often we will be adding/subtracting entire samples from validation.

Edit: The generate-matrix action is the one that would need to be aware of trunk/dev sample set separation. The validate action is told which sample to run and so it will work fine as long as we get the right list of samples.

@tvami
Copy link
Member Author

tvami commented Aug 13, 2024

OK makes sense thanks!

@tvami tvami merged commit f605b6a into trunk Aug 13, 2024
3 of 4 checks passed
@tvami tvami deleted the iss1384-validation-update branch August 13, 2024 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move validation config to 8 GeV + add tracking + intro reduced LDMX config to validation
3 participants