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

use phi size of padplane readout sectors from TPC_FEE_CHANNEL_MAP #3018

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mitrankova
Copy link
Contributor

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work for users)
  • Requiring change in macros repository (Please provide links to the macros pull request in the last section)
  • I am a member of GitHub organization of sPHENIX Collaboration, EIC, or ECCE (contact Chris Pinkenburg to join)

What kind of change does this PR introduce? (Bug fix, feature, ...)

TODOs (if applicable)

Links to other PRs in macros and calibration repositories (if applicable)

@sphenix-jenkins-ci
Copy link

Build & test report

Report for commit 5634ec10eb3fe35ff1d5cd6d5cee2cdcd6d2a7b4:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci
Copy link

Build & test report

Report for commit 579abeed9ac317b1bf25cea92028a3e572267eee:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@pinkenburg
Copy link
Contributor

Hi Mariia, don't worry about the failures - I am using your PR to try a few things. Once I am done I'll merge this

@sphenix-jenkins-ci
Copy link

Build & test report

Report for commit 5530dd303a6b46f046db5b3a2ce4e537bc6541a4:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@pinkenburg pinkenburg requested a review from osbornjd July 28, 2024 00:22
@pinkenburg
Copy link
Contributor

The valgrind error is just a shifted line number of a known issue, it's now in the suppression file

Copy link
Contributor

@osbornjd osbornjd left a comment

Choose a reason for hiding this comment

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

The QA looks fine to me. @mitrankova just to make sure I understand, this puts the pads in the proper location, correct? This is a continuation of what Evgeny had been working on?

@mitrankova
Copy link
Contributor Author

Yes, these modifications correct the max and min sector positions in phi to the ones that are corresponding to the CDB values. However, it doesn't change the residuals much. Probably these are not all the modifications that are needed. I am trying to confirm it now.

@osbornjd
Copy link
Contributor

Okay I will hold off on merging this then. It may be good to double check with Evgeny, as he was seeing a lot of unusual changes in the simulation residuals (but maybe that issue has been resolved)

@pinkenburg pinkenburg marked this pull request as draft August 27, 2024 17:10
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.

3 participants