Skip to content

Conversation

@chchatte92
Copy link
Member

Briefly, what does this PR introduce?

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • New feature (issue #__)
  • Documentation update
  • [ X] Other: __
    -dRICH aerogel refractive index has been changed

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • [X ] Changes have been communicated to collaborators
    @kumardeepraman is aware. We are keeping the older parameters as well, for specific checks to estimate the gain in photon yield.

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

Does this PR change default behavior?

-The reconstructed Cherenkov angle for the aerogel changes from ~193 mrad to ~223 mrad for saturated particles.

@chchatte92 chchatte92 requested a review from veprbl October 3, 2025 12:51
@chchatte92
Copy link
Member Author

@veprbl last night all the checks were done, then this branch differed from the main. So I have merged main into this branch. After all the checks are done, should I just queue to merge into main?
I have another question, maybe of interest for @kumardeepraman as well, in case a change has happened to some paramaters in dRICH, is it mandatory to run all the benchmarking in backward directions or calorimeters? Well, the deviations of main can be of concern, but if the main and the corrections don't differ, the PR will take substantial time before merging. Thanks, Chandra.

@chchatte92 chchatte92 merged commit 3188d2f into main Oct 4, 2025
147 checks passed
@chchatte92 chchatte92 deleted the aerogel_dRICH_1.026 branch October 4, 2025 14:08
@wdconinc
Copy link
Contributor

wdconinc commented Oct 4, 2025

Can you add a reference for the new numbers as a comment in the file? E.g. zenodo link to for the data table with the new measurement.

@chchatte92
Copy link
Member Author

Hi Wouter, here I am attaching the bachelor thesis of Tiziano Boasso who had made the studies
Simulation studies of the dual radiator RICH of the ePIC experiment at EIC https://zenodo.org/records/13254379
@kumardeepraman could you please also attach the slides that you had presented last monday?

Chandra

@chchatte92
Copy link
Member Author

Can you add a reference for the new numbers as a comment in the file? E.g. zenodo link to for the data table with the new measurement.
Sorry I misunderstood. I will do this as well.
Chandra

@veprbl
Copy link
Member

veprbl commented Oct 4, 2025

@veprbl last night all the checks were done, then this branch differed from the main. So I have merged main into this branch. After all the checks are done, should I just queue to merge into main? I have another question, maybe of interest for @kumardeepraman as well, in case a change has happened to some paramaters in dRICH, is it mandatory to run all the benchmarking in backward directions or calorimeters? Well, the deviations of main can be of concern, but if the main and the corrections don't differ, the PR will take substantial time before merging. Thanks, Chandra.

These are good points, and I'm glad we are getting such thoughtful feedback. First, indeed, the requirement for branch to be up to date requires merging changes if a competing PR was merged before yours. We used to have Merge Queue that would automatically do some of those things, but we had to disable it because it did not perform well with how flaky benchmarks produced failed status checks. The issue with running "unnecessary" benchmarks is a valid concern, but fixing that would require to make a system that can figure out a correct subset of benchmarks that needs to be ran. If someone would be interested in working on that, that would be much welcome. Meanwhile, further optimizing benchmark running wall time could directly improve merge, and is a more of a low-hanging fruit.

@chchatte92
Copy link
Member Author

@veprbl last night all the checks were done, then this branch differed from the main. So I have merged main into this branch. After all the checks are done, should I just queue to merge into main? I have another question, maybe of interest for @kumardeepraman as well, in case a change has happened to some paramaters in dRICH, is it mandatory to run all the benchmarking in backward directions or calorimeters? Well, the deviations of main can be of concern, but if the main and the corrections don't differ, the PR will take substantial time before merging. Thanks, Chandra.

These are good points, and I'm glad we are getting such thoughtful feedback. First, indeed, the requirement for branch to be up to date requires merging changes if a competing PR was merged before yours. We used to have Merge Queue that would automatically do some of those things, but we had to disable it because it did not perform well with how flaky benchmarks produced failed status checks. The issue with running "unnecessary" benchmarks is a valid concern, but fixing that would require to make a system that can figure out a correct subset of benchmarks that needs to be ran. If someone would be interested in working on that, that would be much welcome. Meanwhile, further optimizing benchmark running wall time could directly improve merge, and is a more of a low-hanging fruit.

@veprbl Thanks for your elaborated answer. I understood your points and completely share your point of view.
@deepaksamuel are you interested in a "low-hanging fruit" as a starting point? It will be really a good starting point and direct service for the ePIC software community.

@veprbl veprbl added the topic: PID Particle identification label Oct 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

topic: materials topic: PID Particle identification

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants