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

Change tracking timing precision, fixes to DQM histo names #1448

Merged
merged 4 commits into from
Sep 13, 2024

Conversation

tvami
Copy link
Member

@tvami tvami commented Sep 7, 2024

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

What are the issues that this addresses?

Resolves #1402

Check List

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

@tvami tvami added the cleanup label Sep 8, 2024
@tvami tvami requested review from pbutti, bloodyyugo, tomeichlersmith and danyi211 and removed request for pbutti September 8, 2024 16:11
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.

I think this looks good, the reason I'm holding approval is just because this changes the gold in two ways (in the logs and by adding the chi2/ndf histogram to the list of histograms that the current validation comparison tool can handle). Is there anything else we want to merge before changing the gold?

@tvami
Copy link
Member Author

tvami commented Sep 9, 2024

My take is that we can merge this, but not yet make a new patch release / gold. The changes introduced in this PR are exactly taking care of the things that don't work in the comparison, i.e. every validation now fails with (1) not finding the /ndf plots and (2) due to the instability of the tracking timing measurement. By merging this every validation will continuingly fail at these two points, but for a different reason. i.e. it's like a status que until we cut new gold (for which that I agree that it's too early)

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.

I am convinced by your reasoning 👍

@tvami
Copy link
Member Author

tvami commented Sep 9, 2024

I pushed one more cosmetics change in 4af0355
I thought this was not worth another issue / PR

@tvami tvami changed the title Change tracking timing precision, dont use slash in DQM histo name Change tracking timing precision, fixes to DQM histo names Sep 9, 2024
@tvami
Copy link
Member Author

tvami commented Sep 10, 2024

@bloodyyugo are you ok with me changing the tracking DQM histo names? I think this is the only way to have the CI script understand that it's not a directory.

Copy link
Contributor

@bloodyyugo bloodyyugo left a comment

Choose a reason for hiding this comment

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

These all look fine to me.

@bloodyyugo
Copy link
Contributor

Yeah, I was going to change this but you got to it first. Thanks

@bloodyyugo are you ok with me changing the tracking DQM histo names? I think this is the only way to have the CI script understand that it's not a directory.

@tvami tvami merged commit a03100b into trunk Sep 13, 2024
9 of 10 checks passed
@tvami tvami deleted the iss1402-part2-tracking-timing branch September 13, 2024 14:06
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.

Tracking timing changes in log diffs
3 participants