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

Modified .cpp file and adding the parameter section to the xml file #743

Merged
merged 8 commits into from
Jun 25, 2024

Conversation

billlee77
Copy link
Contributor

Briefly, what does this PR introduce?

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • [ x] New feature (issue #__)
  • Documentation update
  • Other: __

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

No

Does this PR change default behavior?

Np

@billlee77 billlee77 linked an issue Jun 4, 2024 that may be closed by this pull request
@billlee77
Copy link
Contributor Author

@kkauder @wdconinc and others, the PR is ready and please double check.

Of course, if you have anything let me know

@github-actions github-actions bot added topic: backward Negative-rapidity detectors (electron-going side) topic: PID Particle identification labels Jun 4, 2024
Copy link
Contributor

@wdconinc wdconinc left a comment

Choose a reason for hiding this comment

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

Why do you read all these variables first into double and then convert them to float?

@billlee77 billlee77 force-pushed the 739-moving-hard-coded-pfrich-parameters-to-the-xml-file branch from a8a311d to efb1ebf Compare June 10, 2024 15:52
@billlee77
Copy link
Contributor Author

@wdconinc The number conversion part is addressed, please take one more look and approve the PR

…' of github.com:eic/epic into 739-moving-hard-coded-pfrich-parameters-to-the-xml-file
@billlee77 billlee77 force-pushed the 739-moving-hard-coded-pfrich-parameters-to-the-xml-file branch from fd570cf to c850e71 Compare June 10, 2024 16:49
@billlee77
Copy link
Contributor Author

We see there is an overlap now, we are working on fixing it.

@billlee77
Copy link
Contributor Author

@wdconinc @kkauder @RomanTkachenko-dev The overlap issue is resolved, please consider merging after the checks.

@kkauder
Copy link
Contributor

kkauder commented Jun 24, 2024

Do we understand the failed checks?

@wdconinc wdconinc requested a review from veprbl June 24, 2024 15:13
@billlee77
Copy link
Contributor Author

@wdconinc Hi Wouter, could you take a look a the auto-check result this time, the only thing that is not finished is the physics benchmark. Is this expected?

Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

Defining constants is not the DD4hep way - those exist in a global namespace. Naming doesn't mention RICHEndcapN_ prefix. Let's merge this anyway.

@wdconinc wdconinc enabled auto-merge June 24, 2024 19:43
@wdconinc wdconinc added this pull request to the merge queue Jun 24, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jun 24, 2024
…743)

### Briefly, what does this PR introduce?


### What kind of change does this PR introduce?
- [ ] Bug fix (issue #__)
- [ x] New feature (issue #__)
- [ ] Documentation update
- [ ] Other: __

### Please check if this PR fulfills the following:
- [ ] Tests for the changes have been added
- [ ] Documentation has been added / updated
- [ ] Changes have been communicated to collaborators

### Does this PR introduce breaking changes? What changes might users
need to make to their code?
No
### Does this PR change default behavior?
Np

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 24, 2024
@wdconinc wdconinc added this pull request to the merge queue Jun 24, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 25, 2024
@kkauder kkauder added this pull request to the merge queue Jun 25, 2024
Merged via the queue into main with commit c899ff9 Jun 25, 2024
115 checks passed
@kkauder kkauder deleted the 739-moving-hard-coded-pfrich-parameters-to-the-xml-file branch June 25, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: backward Negative-rapidity detectors (electron-going side) topic: PID Particle identification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Moving hard coded pfRICH parameters to the xml file
4 participants