feat: update pfrich branch with respect to main#35
Conversation
|
@alexander-kiselev this Could you please do a "smoke test" and make sure your pfRICH usage still basically works on this |
|
Chris,
I will look into this, but cannot promise I do it quickly. What is my deadline?
Cheers,
Alexander.
…________________________________
From: Christopher Dilks ***@***.***>
Sent: Sunday, May 7, 2023 3:22 PM
To: eic/irt ***@***.***>
Cc: Kiselev, Alexander ***@***.***>; Mention ***@***.***>
Subject: Re: [eic/irt] feat: update `pfrich` branch with respect to `main` (PR #35)
@alexander-kiselev<https://github.com/alexander-kiselev> this pfrich-update branch is a "prototype" of the merging of your pfrich branch with main.
Could you please do a "smoke test" and make sure your pfRICH usage still basically works on this pfrich-update branch? Check recent pull requests<https://github.com/eic/irt/pulls?q=is%3Apr+is%3Aclosed> for more details of what has changed on main.
—
Reply to this email directly, view it on GitHub<#35 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ANV7FINPOBLUJWTYZU6QTK3XE7Y6PANCNFSM6AAAAAAXYUMPGU>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
No deadline, I'm also going to try to do some testing with it in EICrecon within the next couple of weeks. I just wanted to make sure recent CMake build changes won't mess up your usage. |
|
@alexander-kiselev This comprises only #ifdef guards and cmake generalizations. I checked that it builds on both my mac and in the EicRoot container. gives a warning "You should update the version to ClassDef(CherenkovPhotonDetector,7)." but so does the pfrich branch. |
|
@alexander-kiselev Could you check and approve when you have a few minutes? |
|
Hi Kolja,
I must be blind and / or dumb today. What is it about? I do not see any issue with any order in this class. Can you come over show me?
Cheers,
Alexander.
…________________________________
From: kkauder ***@***.***>
Sent: Monday, January 22, 2024 1:09 PM
To: eic/irt ***@***.***>
Cc: Kiselev, Alexander ***@***.***>; Mention ***@***.***>
Subject: Re: [eic/irt] feat: update `pfrich` branch with respect to `main` (PR #35)
@alexander-kiselev<https://github.com/alexander-kiselev> Could you check and approve when you have a few minutes?
—
Reply to this email directly, view it on GitHub<#35 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ANV7FINNSH4WA2FJI5BRZVLYP2TO5AVCNFSM6AAAAAAXYUMPGWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBUGU2DCNBYGU>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
These are changes to the pfrich branch that don't do anything to the logic, just clean up with #ifdef's and so on for better integration with the epic stack. |
|
Hi Kolja,
I refer to this one, in particular:
af346a3
Cheers,
Alexander.
…________________________________
From: kkauder ***@***.***>
Sent: Thursday, January 25, 2024 12:22 PM
To: eic/irt ***@***.***>
Cc: Kiselev, Alexander ***@***.***>; Mention ***@***.***>
Subject: Re: [eic/irt] feat: update `pfrich` branch with respect to `main` (PR #35)
These are changes to the pfrich branch that don't do anything to the logic, just clean up with #ifdef's and so on for better integration with the epic stack.
—
Reply to this email directly, view it on GitHub<#35 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ANV7FIODGDNVZC4TE2YBJBDYQKIGDAVCNFSM6AAAAAAXYUMPGWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJQGY3DCMRXGQ>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
C++ loves to throw warnings if members aren't initialized in the exact order listed in the class, that's all. |
|
I know. Just do not understand where from this problem come here, see the respective code in the main branch: https://github.com/eic/irt/blob/main/include/CherenkovRadiator.h .
…________________________________
From: kkauder ***@***.***>
Sent: Thursday, January 25, 2024 2:01 PM
To: eic/irt ***@***.***>
Cc: Kiselev, Alexander ***@***.***>; Mention ***@***.***>
Subject: Re: [eic/irt] feat: update `pfrich` branch with respect to `main` (PR #35)
C++ loves to throw warnings if members aren't initialized in the exact order listed in the class, that's all.
—
Reply to this email directly, view it on GitHub<#35 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ANV7FIIYWHKRBMT5KUH4NW3YQKT2LAVCNFSM6AAAAAAXYUMPGWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJQHAYTAMBTGY>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
Remember, this isn't a PR into main but into pfrich. In that branch, you deleted m_ID and Chris restored it (not sure what he needs it for), but in some other place. |
|
OK, then I guess I don't care?
…________________________________
From: kkauder ***@***.***>
Sent: Thursday, January 25, 2024 3:09 PM
To: eic/irt ***@***.***>
Cc: Kiselev, Alexander ***@***.***>; Mention ***@***.***>
Subject: Re: [eic/irt] feat: update `pfrich` branch with respect to `main` (PR #35)
Remember, this isn't a PR into main but into pfrich. In that branch, you deleted m_ID and Chris restored it (not sure what he needs it for), but in some other place.
—
Reply to this email directly, view it on GitHub<#35 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ANV7FIMULHZ5BY2TVGJ7XL3YQK3YBAVCNFSM6AAAAAAXYUMPGWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJQHEYTGNJSGI>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
Reviewing, yes, that was my takeaway. I don't see a need for that m_ID, so we can accept it or not, but it didn't move the needle for significance for me. You feel strongly one way or the other, let's do that. @c-dilks , can you add to the need for |
At least in
It seems this is only used for log printouts, so it may be safe to remove. |
Briefly, what does this PR introduce?
Merging
pfrichintomainis not straightforward and risks breaking dependent code. Therefore, let's first mergemainintopfrich, and make sure all important patches are applied to all new files:mainand resolve conflictscmakeconfig #13