TRD: PID: fixups #11664
Merged
TRD: PID: fixups #11664
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
f3sch
requested review from
bazinski,
tdietel,
martenole and
pachmay
as code owners
|
Thanks, did not go through it yet but the CI errors are real, can you please check? |
martenole
reviewed
Jul 24, 2023
martenole
reviewed
Jul 24, 2023
| { | ||
| /// Lookup Table class for ccdb upload | ||
| template <int nDim> | ||
| class LUT |
There was a problem hiding this comment.
I like the way to have the LUT as a class with a configurable number of dimensions. Would it be possible to make it accept not only 1 and 3D, but also things like 2 or 7D for example? I think we had such LUTs in the past in Run 2
There was a problem hiding this comment.
7D seems nonsensical since we only get 3 windows from the FEE anyways, this was different in Run 2 were we would get 7 windows, I think.
How would you combine the windows in the 2D case (Q0+Q2 and Q1, seems the most resonable to me?)
There was a problem hiding this comment.
ok 7D is probably indeed not needed, but 2D I could imagine (although no idea which windows would be best at the moment). Could you not get rid of the whole if constexpr block of the class? For 1D it will anyhow be called with iDim = 0 only and so you could return mLUTS[index + iDim] always, no?
|
Error while checking build/O2/fullCI for c6ccbde at 2023-08-01 11:50: Full log here. |
|
Error while checking build/O2/fullCI for 2eb2ae0 at 2023-08-01 17:19: Full log here. |
|
Error while checking build/O2/fullCI for d78aebd at 2023-08-02 09:53: Full log here. |
|
Error while checking build/O2/fullCI for 44c2c97 at 2023-08-02 11:57: Full log here. |
|
Error while checking build/O2/fullCI for cbb89aa at 2023-08-05 03:36: Full log here. |
This commit renames the factory function to better reflect its purpose. Additionally, z-row merging and charge correction have been added to the codebase to improve functionality. Pytorch policy and LQND policy have been added as new policies. The README has also been added to provide additional explanation of the code. As part of this update, the pid policy map and pidvalue alias have been removed. A print overload has been added for the policy enum to improve readability. Various minor fixes have also been made to improve overall code quality. Also included are various changes to the class layout and the ccdb object for LUTs. Signed-off-by: Felix Schlepper <[email protected]>
martenole
approved these changes
Aug 10, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@martenole For you to look over.
I am not sure what way would be prefered for the lookup tables, just a vector and hardcode the momentum intervals or the class I included?
Also I did not change the paths to the ccdb yet, we can upload a preliminary object or wait for the results of the service work by leonardo?
And should I pull over the macros from the other repo so that we have the CI?