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

Qatable #13633

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

Qatable #13633

wants to merge 5 commits into from

Conversation

f3sch
Copy link
Collaborator

@f3sch f3sch commented Oct 29, 2024

Copy link
Contributor

REQUEST FOR PRODUCTION RELEASES:
To request your PR to be included in production software, please add the corresponding labels called "async-" to your PR. Add the labels directly (if you have the permissions) or add a comment of the form (note that labels are separated by a ",")

+async-label <label1>, <label2>, !<label3> ...

This will add <label1> and <label2> and removes <label3>.

The following labels are available
async-2023-pbpb-apass4
async-2023-pp-apass4
async-2024-pp-apass1
async-2022-pp-apass7
async-2024-pp-cpass0

@miranov25
Copy link
Contributor

image

@f3sch
Copy link
Collaborator Author

f3sch commented Nov 12, 2024

undrafting to check CI
Do not merge until @miranov25 completes the closure tests.
Converter AliceO2Group/O2Physics#8404

@miranov25
Copy link
Contributor

Thank you for following.
We are still fixing digital signal processing, so I have to finish other stuff.
I haoe I will manage to check it this week, as I want to have it soon in productions.

@miranov25
Copy link
Contributor

Internal information - test at GSI using debug streamer:

rsync -avzt --progress [email protected]:/afs/cern.ch/user/f/fschlepp/public/output.tar.gz 
 /lustre/alice/users/miranov/NOTESData/alice-tpc-notes/JIRA/O2-5095/testDelta

@miranov25
Copy link
Contributor

Hello @f3sch,

Thank you for implementing the changes. I have identified two major issues so far:

  1. Coding Resolution: In my proposal described during the AOT meeting, I suggested using coding with 10-25% resolution. However, you implemented 1-sigma coding, which is insufficient.

  2. dE/dx Scaling Issue: I will provide a detailed description in the next comment.

Additionally, I would recommend using consistent variable names in both the source code and the debug stream to make it easier to test and ensure code consistency.

The code for testing I committed to the tpc gitlab, I am not sure if we have some code for the O2 testing.

I produced QA plots with that.

Thank you for your help

Marian

image

@miranov25
Copy link
Contributor

miranov25 commented Nov 19, 2024

Track Residual Coding:

  • As I wrote earlier, we will need a few samples for delta coding—for this, I recommend using 5 samples per sigma, as shown in the slide.
  • In the current code, the Beta value was incorrectly coded, leading to an underestimation of the error for high dE/dx (low Beta) particles.
  • code as in the pull request
const float beta0 = std::min(tpcOrig.getdEdx().dEdxMaxTPC / 50.f, 1.f);
  • should be:
    tree->SetAlias("betaC","sqrt(min(50./mdEdx.dEdxMaxTPC, 1.))");

After the fix, the pulls—normalized errors—are now close to one (as should be) as shown in the slide.

Test code used in slide:

image

@f3sch
Copy link
Collaborator Author

f3sch commented Nov 19, 2024

@miranov25 at your leisure, I updated the code and uploaded updated files for you to test at the path posted above.

@miranov25
Copy link
Contributor

Hello @f3sch,

Thank you very much for making the changes. I see you have modified the Beta0 estimator as we discussed and agreed upon.

However, there is another adjustment needed related to the sampling. In the current code, you are using sampling with 1 sigma resolution, but we need to sample it with 5 bins per sigma (to be able to correct later)

I will add the proposed modification directly to the code, temporarily as a hardcoded number.

Additional modifications needed

  • Use sampling of sigma/5 as I described above.
  • Ensure consistent variable names between the code and the streamer, so the same variables are reflected in the test code.
  • Include the expected sigma in the output, so it does not need to be defined as an alias in the test code, so test will be easier to interpret.

For the sampling of sigma/5, we should see approximately 5 bins in dRefContY, but currently, we only have 1 bin.
In the example I use case of the dY for the TPCITS but it is similar in all other cases.

Best regards
Marian

   tree->Draw("trackQAHolder.dRefContY>>his(100,-50,50)","itstpc_dP0!=0","");

image

@miranov25
Copy link
Contributor

For now, I suggest modifying the scaleCont and scaleGlo functions by using nBins as a parameter.

A more elegant approach would be to create sigmaCont and sigmaGlo functions, and then apply scaling using (sigma<...>)/nBins in the subsequent code. Like that the variables and code will be self documented

@f3sch
Copy link
Collaborator Author

f3sch commented Nov 20, 2024

@miranov25 I do not understand, please send me the code you want to have changed, or add it here as a commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants