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

8GeV config added for inclusive sample #1272

Merged
merged 3 commits into from
Apr 10, 2024

Conversation

pritampalit
Copy link

@pritampalit pritampalit commented Mar 20, 2024

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

Check List

  • I successfully compiled ldmx-sw with my developments

  • I ran my developments and the following shows that they are successful.

Link to presentation

  • I attached any sub-module related changes to this PR.

To run this config :

ldmx fire .github/validation_samples/inclusive/config_inclusive_8gev.py [run_number] [max_events] [max_TriesPerEvent]

Copy link
Contributor

@bryngemark bryngemark left a comment

Choose a reason for hiding this comment

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

I think we don't want a new file called ..._8gevbut actually an update to the existing file. 8 GeV is our new default and this should be reflected in our CI configs. Changing the original file also makes it easier to review your exact changes in the pull request.

@bryngemark
Copy link
Contributor

by the way i expect the associated test to break if you rename the existing file, since two different beam energies are compared. that is ok. it would be good to just take a quick look at the resulting histograms ("artifacts") that the diffrences are expected, just as you have done for validation.

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.

Besides a niche output-filename change that is required for the CI, I can't think of anything else to change. As @bryngemark said, this will fail the PR Validation histograms but those histograms will be interesting to look at.

.github/validation_samples/inclusive/config.py Outdated Show resolved Hide resolved
@tomeichlersmith tomeichlersmith marked this pull request as draft March 30, 2024 16:25
@tomeichlersmith tomeichlersmith marked this pull request as ready for review March 30, 2024 16:25
@tvami tvami added the config label Apr 8, 2024
@tvami
Copy link
Member

tvami commented Apr 9, 2024

I think this needs to go in before we change the CI configs to add the debug level info as discussed in #1277 (comment)

@tomeichlersmith tomeichlersmith merged commit 9d0a222 into LDMX-Software:trunk Apr 10, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants