Skip to content

Ilbelov 3pi fits - #417

Open
iljatt wants to merge 5 commits into
masterfrom
ilbelov_3pi_fits
Open

iljatt wants to merge 5 commits into
masterfrom
ilbelov_3pi_fits

Conversation

@iljatt

@iljatt iljatt commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

Amplitudes will work on trees with merged diamond orientations. The two necessary parameters (polarization fraction and diamond orientation angle) are either defined in the config file and passed to the amplitude as external parameters, or read from the input trees as px and py components of the beam four-vector.

@gluex

gluex commented Sep 28, 2026

Copy link
Copy Markdown

Build status for this pull request: FAILURE

Build log: make_ilbelov_3pi_fits.log
Build report: report_ilbelov_3pi_fits.txt

@gluex

gluex commented Sep 28, 2026

Copy link
Copy Markdown

Build status for this pull request: FAILURE

Build log: make_ilbelov_3pi_fits.log
Build report: report_ilbelov_3pi_fits.txt

@gluex

gluex commented Sep 28, 2026

Copy link
Copy Markdown

Test status for this pull request: SUCCESS

Summary: summary.txt
Logs: results/log

Build log: make_ilbelov_3pi_fits.log
Build report: report_ilbelov_3pi_fits.txt

@mashephe

mashephe commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

This is a good strategy for handling the polarization in merged trees. However, it is worth noting at the convention is different than the strategy used elsewhere. Fore example see line 87 of Zlm.cc -- here the 3 elements of the beam four-vector are used to hold elements of a vector that points in the direction of the polarization with a magnitude equal to the magnitude of the polarization: ( P cos( polAngle ), P sin( polAngle ), 0 ). It might be helpful to standardize the approach across amplitude classes to avoid confusion.

Also note that for fits with large numbers of amplitudes, this "division of labor" between calcUserVars and calcAmplitude puts a lot of load on the calcUserVars method. If you like doing GPU fits, this may leave the GPU under-utilized because user variables are all computed on the CPU. In general there is no "one-size fits all" optimization here because it depends on the hardware you are using, where free parameters show up, etc. I don't consider this an issue or problem that needs resolution before merging, I just wanted to point it out.

@mashephe

Copy link
Copy Markdown
Contributor

I understand also that implementing my comment above requires reformatting root trees. Perhaps this is a pain right now and there is urgency just to get the amplitude in the main code base so that MC studies could be done with the OSG framework. If that is the case - no problem. I can approve this and we can put unifying the beam polarization representation as a "to-do" for the future.

@iljatt

iljatt commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

Many thanks for review and your comments, Matt.

I think, we can postpone merging to the master branch then. I will reformat my trees and update the amplitudes. Then I will also have to update the decayAngles amplitude, I would like to overload the getPhiProd() function with analogous function which receives epsilon-vector instead of polAngle.

@mashephe

Copy link
Copy Markdown
Contributor

OK - I wasn't sure if you really needed this amplitude and its registration in a production release to be able to efficiently use it.

Simultaneously @amschertz is working on a revisions of probably what are very similar decay angles codes for the b1 fitting. It might be good to try to coordinate this a bit. Amy had a few goals in her revisions aimed at performance optimizations. For her fits, going to the GPU is not beneficial for the reasons I noted above. We were trying to make that a bit more flexible.

@iljatt

iljatt commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

The thing is that I didn't compare efficiency for this amplitude between running fits on CPUs and on GPU.
I am working now on cluster in Germany. I couldn't make AmpTools work efficiently on GPUs, since my fits on GPUs work much slower than the same fits on CPUs. Therefore I went forward with using CPUs and came to this optimization, which gives me speed up factor from 3 to 9 for my m(3pi)-binned fits (depending on the number of events in the likelihood), when running fits on 32 CPUs. Anyway, the message is that I didn't test impact of this optimization on fitting with GPUs.

@iljatt

iljatt commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Perhaps, we will have to figure out if we can benefit from switching to GPUs on our cluster.
And we may need your advice at some point.
If option with GPUs doesn't work, I will prefer to implement this optimization.

@mashephe

Copy link
Copy Markdown
Contributor

I saw similar issues with Amy's production fits on GPUs. I just sent you by email some notes that I posted to slack about a month ago. If your fit time is spent in calcUserVars then GPU is not going to help since that is done on CPU by design. To make GPU work efficiently you might structure the fit so user variables are "static" (they can be reused by every instance of the amplitude). That shifts compute load to the calcAmplitude, which is parallelized on GPU. The MPI implementation will parallelize everything, including calcUserVars. GPU is probably best if you are floating parameters in amplitudes in the fit.

I'm happy to help with optimizing as I can. I think one thing that would be helpful is to put some timers in the fitting function. There are generally 3 blocks that you might be interested in timing:

  1. the first likelihood computation
  2. the minimization time to convergence, including average time per function call (this is already printed to the screen)
  3. the call to AmpToolsInterface::finalizeFit, which ultimately triggers a computation of the normalization integrals on the generated MC set

For Amy's production fit, the most expensive of these was (3). That step is accelerated by MPI but not significantly by GPU. So, one needs to look at that computation and how to do it most efficiently.

Let me know if I can help more.

@bgrube

bgrube commented Sep 29, 2026

Copy link
Copy Markdown

It might be worth considering running GPU jobs at the JLab Farm. It offers in total 28 A800 cards and Alex could arrange prioritized access.

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.

4 participants