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

Fix deprecated functions in ONNX and boost + type conversion for ConfigurePythonTest #1393

Merged
merged 1 commit into from
Aug 21, 2024

Conversation

tvami
Copy link
Member

@tvami tvami commented Aug 19, 2024

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

What are the issues that this addresses?

This resolves #1208

Check List

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

@tvami
Copy link
Member Author

tvami commented Aug 19, 2024

@tomeichlersmith what do you think about what I did here for boost? This (partially) solves the warning (the <iterator> part I cannot do anything about), but I think it's also the reason for the change in the logs. I could remove that from this PR and have you bump up the boost version and come back to this later. Or keep it as is and revisit this warning whenever the boost update happens.

@tvami tvami marked this pull request as ready for review August 19, 2024 19:50
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 makes sense. The deprecation methods within Boost are from the Boost.Python library which we use for the bindings within DetDescr. I'm not super worried about the text changes to the log [0] since the changes are only on a handful of events out of 10k. Maybe the change to Boost is somehow affecting the TDecompSVD class in ROOT? It seems to be an edge case at least.

All I'm worried about is the ONNX change since I'm so unfamiliar with it, but a few test-runs with someone more familiar with the BDT would be able to confirm that GetShape and GetDimension are equivalent for us.

[0] Changes to logs were in the signal validation sample when PR was originally opened.
  <  [ ecalVeto ] 0 :   MIP tracking completed; found 4 straight tracks and 0 lin-reg tracks
  ---
  >  [ ecalVeto ] 0 :   MIP tracking completed; found 4 straight tracks and 2 lin-reg tracks
  13163c13163
  <  [ ecalVeto ] 0 :   MIP tracking completed; found 0 straight tracks and 0 lin-reg tracks
  ---
  >  [ ecalVeto ] 0 :   MIP tracking completed; found 0 straight tracks and 1 lin-reg tracks
  73962c73962
  <  [ ecalVeto ] 0 :   MIP tracking completed; found 2 straight tracks and 0 lin-reg tracks
  ---
  >  [ ecalVeto ] 0 :   MIP tracking completed; found 2 straight tracks and 1 lin-reg tracks
  101379c101379
  <  [ ecalVeto ] 0 :   MIP tracking completed; found 3 straight tracks and 0 lin-reg tracks
  ---
  >  [ ecalVeto ] 0 :   MIP tracking completed; found 3 straight tracks and 1 lin-reg tracks

@tomeichlersmith
Copy link
Member

tomeichlersmith commented Aug 20, 2024

Alright, the change in lin-reg tracks within the ECal Veto is still persisting. Here is a quick summary,

Sample Change
it_pileup 4 Events: 2 -> 0; 43 Events: 1 -> 0
reduced_ldmx 1 Event: 0 -> 1
inclusive Does not run EcalVeto
hcal Does not run EcalVeto
ecal_pn No Change (probably due to change in log level) 1
signal 1 Event: 0 -> 1; 3 Events 0 -> 2

While I am mildly curious about the cause behind this, I think it could reasonably be from a change in the underlying runner's software (OS or version of docker for example) that causes a slight change for us.

Footnotes

  1. The ecal_pn fails validation since the logs change by a few characters when reporting the timing of different Tracking components at the end of processing.

@tvami
Copy link
Member Author

tvami commented Aug 20, 2024

No MIP Tracking Change

I'm not sure that's just not hidden given that the log level is different from the rest of them :/

@tvami
Copy link
Member Author

tvami commented Aug 20, 2024

OS or version of docker for example

Where did you do this? This LDMX-Software/docker#97 you closed, right?

@tomeichlersmith
Copy link
Member

No I mean the OS of the runner launched by GitHub. I'd be surprised if that affects things, but throwing stuff out there.

You're right about the no change probably being from the log level.

@tomeichlersmith
Copy link
Member

I tried running locally and the log diff showed even more differences. I need to check my local configuration and stuff but I fear that this is not something that is easy to reproduce and test.

@tomeichlersmith
Copy link
Member

@tvami I see two ways forward:

  1. Just Merge -- I think this isn't too big of a deal and encourages us to double check the MIP track counts (which we should probably do anyways).
  2. Separate Changes into Different PRs -- This would then allow us to see if there is a single change that is causing the change in MIP track counts and then we could go from there.

What do you think?

@tvami
Copy link
Member Author

tvami commented Aug 20, 2024

I still havent had the chance to look at the BDT score and other plots in higher stats. It's not a lot of work, I just keep getting distracted with other things :D But yeah let me post some plots by tomorrow. If you want, you can approve based on the code changes, and then I'll post the plots and we can merge later if they look good

@tvami
Copy link
Member Author

tvami commented Aug 21, 2024

CompareBDT

The BDT values are exactly the same for ECAL pn, which makes me happy with the changes.
I also looked at the nStraightTracks and the nLigRegTracks, they also look identical.

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.

Sweet, thanks for doing that double check. I feel mich more confident that the log changes are neglible.

@tomeichlersmith tomeichlersmith merged commit cad1ed8 into trunk Aug 21, 2024
15 of 16 checks passed
@tomeichlersmith tomeichlersmith deleted the iss1208-compiler-warnings branch August 21, 2024 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiler warnings
2 participants