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

Clarify that ReducedBeamCharacteristics are relative to reference particle #585

Merged

Conversation

n01r
Copy link
Member

@n01r n01r commented Apr 18, 2024

Make clear how the ReducedBeamCharacteristics quantities are in reference to the reference particle.

Make clear how the ReducedBeamCharacteristics quantities are in reference to
the reference particle.
@n01r n01r requested review from ax3l and SchroederSa April 18, 2024 18:34
@n01r
Copy link
Member Author

n01r commented Apr 18, 2024

This PR addresses #533.
@SchroederSa, let me know if you'd prefer it more explicit or differently phrased

@ax3l ax3l added the component: documentation Docs, readme and manual label Apr 23, 2024
@ax3l ax3l requested a review from cemitch99 April 23, 2024 18:10
@ax3l ax3l self-assigned this Apr 23, 2024
Co-authored-by: Chad Mitchell <46825199+cemitch99@users.noreply.github.com>
@ax3l ax3l requested a review from cemitch99 April 24, 2024 17:53
Copy link
Member

@cemitch99 cemitch99 left a comment

Choose a reason for hiding this comment

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

This looks clear to me.

Copy link
Contributor

@SchroederSa SchroederSa left a comment

Choose a reason for hiding this comment

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

Thanks Marco! That makes it much clearer for sure. And the coordinates and units section is marvelous! (Maybe the Theory section in the docs should come right after Installation and well before the Usage section - note to myself :D)
I added some thoughts.
Sarah

@@ -59,6 +59,7 @@ Reduced Beam Characteristics
----------------------------

ImpactX calculates reduced beam characteristics like averaged positions, momenta, beam emittances and Courant-Snyder (Twiss) parameters during runtime.
Copy link
Contributor

@SchroederSa SchroederSa Apr 25, 2024

Choose a reason for hiding this comment

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

Would it make sense to state in this sentence that this is related to diag.enable, i.e. the 'diag.' section in the input file docs? I remember I was confused by what this section is about, where the reduced diags is specified in the program and actually the whole diagnostics setup. The third sentence where 'diag.slice_step_diagnostic' is mentioned gives a glimpse but describes a detail. (Likewise an explicit pointer to <element_name>.beam_monitor would be helpful in the section above.)

I think what contributes to the confusion is the subtle difference in terminology and a fundamental difference to the other codes (WarpX/HiPACE++). In this section it is called 'reduced beam diagnostics' but in the inputs description it is called just 'diagnostics' while the holistic 6D diagnostic of the beam can only be done through monitors, which are not specified through the 'diag.' inputs and are not explicitly mentioned in the diagnostics section. While for the other codes with 'diag.' the holistic diagnostic is specified and the in-situ (~reduced) diagnostic appears as an additional feature within the 'diag.' specifications. I guess, this follows the idea, that the reduced diagnostics is the relevant diagnostic but then the changing terminology is confusing :).

Maybe adding a sentence in the 'Parameters: Inputs File/Diagnostics and output' section that clarifies the two options of diagnostics and access points would be helpful? Right now the headline 'Diagnostics and output' kinda infers that 'diag.' specifies THE diagnostic but as much as I understand it mainly relates to the reduced beam diagnostic and 6D phase space openPMD output is generated via the elements. The latter can only be passively grasped when digesting the side notes: 'This option is ignored for the openPMD output elements (remove them from the lattice to disable).' and 'See the beam_monitor element for backend values.'.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks Sarah, that is a good point.

Before adding the monitors we had some ad-hoc diagnostics (in ASCII) in ImpacX, where some of the legacy wording here comes from. We do not have a "full" diagnostics outside of monitors in ImpactX, for simplicity (still needs #537).

The "reduced diagnostics" (in situ diagnostics) is now also added to the monitors #584, so that we can technically streamline this even a bit more.

Let's add the cross-section hints you propose for now, to clarify what means what.

Copy link
Member

Choose a reason for hiding this comment

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

I made an edit to line 61, so these comments now are invisible under the Files Changed tab. Just FYI - these are good comments and we don't want to lose them.

@@ -59,6 +59,7 @@ Reduced Beam Characteristics
----------------------------

ImpactX calculates reduced beam characteristics like averaged positions, momenta, beam emittances and Courant-Snyder (Twiss) parameters during runtime.
Averaged positions and momenta are given as deviations with respect to the reference particle (see `Coordinates and Units <theory-coordinates-and-units>`_).
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe 'deviation' is a dangerous word here due to its mathematical definition but also plain language usage. Wouldn't just 'Positions and momenta are given with respect to the reference particle' be less confusing also because they are treated differently?

Also: it's not only the averaged position and momenta but also minimum and maximum.

Copy link
Member

Choose a reason for hiding this comment

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

I think deviation is correctly meant in the mathematical way (difference) here, no? @cemitch99

There is a small linkage issue here though

Suggested change
Averaged positions and momenta are given as deviations with respect to the reference particle (see `Coordinates and Units <theory-coordinates-and-units>`_).
Averaged positions and momenta are given as deviations with respect to the reference particle (see :ref:`Coordinates and Units <theory-coordinates-and-units>`).

Copy link
Member

Choose a reason for hiding this comment

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

I agree with Sarah's comment about the use of "Averaged" here. I placed an alternative suggestions above.

docs/source/dataanalysis/dataanalysis.rst Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

You changed the wording from 'beam momentum' / 'beam position' etc to 'particle momentum'. Is there a particular reason for it? The section title is 'beam characteristic'. I am confused :)

Copy link
Member

Choose a reason for hiding this comment

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

That was me, sorry. I did this because the word "beam" usually implies a collective property, like "beam energy" or "beam size". At this point in the documentation, we are referring to the individual particle position and momentum. (Of course, after the reduced diagnostics are computed, those are properties of the beam--hence the section title.) Is that too confusing?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine! :)

cemitch99 and others added 2 commits April 26, 2024 14:55
Co-authored-by: SchroederSa <134732256+SchroederSa@users.noreply.github.com>
Co-authored-by: Chad Mitchell <46825199+cemitch99@users.noreply.github.com>
Copy link
Member

@ax3l ax3l left a comment

Choose a reason for hiding this comment

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

Thank you all! :)

@ax3l ax3l merged commit b43609d into ECP-WarpX:development May 1, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: documentation Docs, readme and manual
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants