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

Fairmat 2024: additions and clarifications in NXbeam #1408

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

lukaspie
Copy link
Contributor

@lukaspie lukaspie commented Sep 23, 2024

With this PR, the idea is:

  • to clarify how the incident_energy field in NXbeam is to be used. Specfically, for polychromatic beams, a single field incident_energy is not sufficient, but rather one needs information about the incident_energy_spread. In addition, for a polychromatic beam, it is useful to incidate the incident_energy_weights, i.e., how much each each of energies contributes to the overall intensity.
  • add some fields about pulsed beams and allow for descriptions of FROG (frequency-resolved optical gating) setups.
  • add a field previous_device to indicate that this beam originate from a given beam device. This can be useful to describe the chain of devices and beams in the description of the beam paths (e.g., in an optical spectroscopy experiment).

@sanbrock sanbrock mentioned this pull request Sep 26, 2024
1 task
@lukaspie lukaspie linked an issue Sep 29, 2024 that may be closed by this pull request
1 task
@prjemian
Copy link
Contributor

RST wraps computer code with double backticks:

* correct: ``energy``
* incorrect: `energy`

This is one of the most common typos in the documentation.

@lukaspie
Copy link
Contributor Author

RST wraps computer code with double backticks:

* correct: ``energy``
* incorrect: `energy`

This is one of the most common typos in the documentation.

You are right, I changed it here for our new contribution where we make use of backticks.

Copy link
Contributor

@RubelMozumder RubelMozumder left a comment

Choose a reason for hiding this comment

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

I just randomly went through the file to be knowledged of the NAIC decision. I found some typos of bouble back tick while mentioning some fields.

base_classes/NXbeam.nxdl.xml Outdated Show resolved Hide resolved
base_classes/NXbeam.nxdl.xml Outdated Show resolved Hide resolved
base_classes/NXbeam.nxdl.xml Outdated Show resolved Hide resolved
@lukaspie lukaspie force-pushed the fairmat-2024-nxbeam branch from 2529db0 to c75d5c2 Compare October 16, 2024 08:44
@lukaspie
Copy link
Contributor Author

lukaspie commented Dec 6, 2024

@phyy-nx I added a bit of description to this PR as well. As far as I can remember, there was a bit of discussion on this PR in the NIAC meeting, but no formal decision was taken. This PR is ready in so far as further discussion (and subsequent decision) by NIAC can be initiated.

Note that when going through this PR again, I figured it could also be useful to extract the characteristics of a pulsed beam (which make up most of the changes in this PR) to its own sub-class. We would be open to doing so if that is what NIAC wants.

@phyy-nx
Copy link
Contributor

phyy-nx commented Dec 9, 2024

Comments from Telco:

  • Instead of duplicating documentation from incident_wavelength in incident_energy, supply a reference instead and note the same conventions. The same for incident_energy_weights and incident_energy_spread.
  • Change units back to NX_ANY for incident_polarization and final_polarization. Rationale: neutrons are specified as a percentage of spin states, so we can't constrain it here.

Once done, based on Telco conversation, we can put this up for an online vote

@phyy-nx
Copy link
Contributor

phyy-nx commented Dec 10, 2024

Hi all, as discussed in the Telco, now that the above points have been addressed (thanks @lukaspie), we can now move this PR to an online vote.  NIAC committee members please vote on this PR using emojis. 👍 for yes, 👎 for no, anything else (for example 👀) to abstain.  We need 14 votes to hit quorum so please review and vote!

As you review, please make a special note of the proposal to add these fields:

    <field name="previous_device">
        <doc>
             Indicates the beam device from which this beam originates.
             This defines, whether the beam in an "input" or "output" beam.
        </doc>
    </field>
    <field name="next_device">
        <doc>
             Gives the beam device which this beam will interact with next.
        </doc>
    </field>

As discussed on the call, these two fields allow connecting devices together. A figure was shown demonstrating the usage. We decided that the depends_on convention used for NXtransformations wasn't applicable here, due to the possibility of a set of branched connections.

Any questions we can answer here. Otherwise, cast a vote! Thanks!

@lukaspie
Copy link
Contributor Author

As a follow up to the discussion in the Telco, here is the image that I was showing on how we are envisioning connecting devices and instances of NXbeam together.
image

@lukaspie lukaspie removed discussion needed NIAC should review The NIAC should review/discuss labels Dec 11, 2024
@PeterC-DLS
Copy link
Contributor

Can the previous/next thing be an NXsample? Maybe a better name is required than device

@phyy-nx
Copy link
Contributor

phyy-nx commented Dec 18, 2024

Can the previous/next thing be an NXsample? Maybe a better name is required than device

My thought was that this is for components hooked together. The only usage of NXbeam at present is in NXmx and it is a member of an NXinstrument group. The implication is that beam hits the sample. Indeed, NXmx used to have the NXbeam group in the NXsample group, but we moved it to the NXinstrument group instead.

@sanbrock @mkuehbach @lukaspie is connecting a beam to a sample directly a use case you are interested in?

Copy link
Contributor

@phyy-nx phyy-nx left a comment

Choose a reason for hiding this comment

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

Couple typos (thanks @benajamin)

base_classes/NXbeam.nxdl.xml Outdated Show resolved Hide resolved
@lukaspie
Copy link
Contributor Author

lukaspie commented Dec 19, 2024

Can the previous/next thing be an NXsample? Maybe a better name is required than device

My thought was that this is for components hooked together. The only usage of NXbeam at present is in NXmx and it is a member of an NXinstrument group. The implication is that beam hits the sample. Indeed, NXmx used to have the NXbeam group in the NXsample group, but we moved it to the NXinstrument group instead.

@sanbrock @mkuehbach @lukaspie is connecting a beam to a sample directly a use case you are interested in?

Indeed, we mainly focused on hooking together components. But, having an NXsample as the previous/next thing would be also something that we could envision. An example would be the beam path for optical spectroscopy experiments, where several beams are connecting different devices (see the use case above), but then they could also encounter a sample along the way.

I think we would be open to remaining to something more general like previous_object or previous_beam_object if that's helpful. Not sure if that would needs its own vote or if the current vote must be amended in this case.

Also tagging in @RonHildebrandt here.

@tacaswell
Copy link
Contributor

tacaswell commented Dec 23, 2024

I apologize for missing the telco and possibly asking redundant questions:

  • what does This defines, whether the beam in an "input" or "output" beam. mean? Should this be understood as implying that previous_device / next_device are mutually exclusive?
  • next_device vs next_element? Will there ever be a case where someone will want to track beams internal to something they call a "Device" (because it is all in one vacuum chamber or just to match their local jargon)?
  • what about beams that can hit multiple down-stream elements? Maybe a beam with a big spacial spread that is then hit by multiple detectors or elements (like a bpm or ion chamber) that are not expected to significantly impact the beam as it passes.

@lukaspie
Copy link
Contributor Author

lukaspie commented Jan 6, 2025

I apologize for missing the telco and possibly asking redundant questions:

  • what does This defines, whether the beam in an "input" or "output" beam. mean? Should this be understood as implying that previous_device / next_device are mutually exclusive?

No, these are not meant to be mutually exclusive. Rather, they should be used together to achieve the chaining as shown in the image above. Maybe this sentence could be removed, what do you think?

  • next_device vs next_element? Will there ever be a case where someone will want to track beams internal to something they call a "Device" (because it is all in one vacuum chamber or just to match their local jargon)?

next_element probably is the best name, that would also work for me/us. I am not sure if such a renaming would be covered by the existing vote though.

  • what about beams that can hit multiple down-stream elements? Maybe a beam with a big spacial spread that is then hit by multiple detectors or elements (like a bpm or ion chamber) that are not expected to significantly impact the beam as it passes.

This is indeed something that was not really considered so far. I see two ways to cover this:

  1. split the beam into multiple beam instances, each with a different next_device (or next_element).
  2. allow the previous*/next* elements to be a list of devices.

I think option 1) would be the cleanest solution.

@ggoneiESS
Copy link

RE: option 1/2 above; would you then envisage that a neutron beamline with N choppers and M beam monitors would have N+M+1 NXbeam instances between source and sample?

@phyy-nx
Copy link
Contributor

phyy-nx commented Jan 6, 2025

13 votes isn't enough to pass, which is fine due to the continuing discussion. We'll review in the next Telco.

This defines whether the beam is an "input" or "output" beam.
</doc>
</field>
<field name="next_device">
Copy link
Contributor

@woutdenolf woutdenolf Jan 6, 2025

Choose a reason for hiding this comment

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

I'm assuming previous_device and next_device are NX_CHAR with the full path of a NeXus group? We probably want to clarify this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lukaspie lukaspie force-pushed the fairmat-2024-nxbeam branch from da81fb9 to 3599458 Compare January 16, 2025 15:59
@phyy-nx
Copy link
Contributor

phyy-nx commented Jan 16, 2025

Proposal from Telco:

  • Remove previous/next device from this PR
  • Remove NXbeam_element from this PR
  • Add inputs and outputs to NXcomponent: lists of other components (NX_CHAR, rank of 0 or 1 (single or multiple), but it's optional)

Implies that NXcomponent would include anything that can affect the beam

A charge to the committee: go review and comment on #1525!! (consider implications of #1507)

Would want a flow/class diagram showing this inheritance. Also, don't want to lose the cool figure above of the multiple beam paths!

domna and others added 12 commits January 17, 2025 14:34
… version of yaml.

Removing unintensional comments
* Add nexus definitions/files for beam path description

* Update base_classes/nyaml/NXopt_assembly.yaml

Co-authored-by: Lukas Pielsticker <[email protected]>

* Update base_classes/nyaml/NXopt_assembly.yaml

Co-authored-by: Lukas Pielsticker <[email protected]>

* Apply suggestions from code review

Co-authored-by: Lukas Pielsticker <[email protected]>

* Apply suggestions from code review

Co-authored-by: Lukas Pielsticker <[email protected]>

* Apply suggestions from code review

Co-authored-by: Lukas Pielsticker <[email protected]>

* Apply suggestions from code review

Co-authored-by: Lukas Pielsticker <[email protected]>

* Apply suggestions from code review

Co-authored-by: Lukas Pielsticker <[email protected]>

* Apply suggestions from code review

Co-authored-by: Lukas Pielsticker <[email protected]>

* Apply suggestions from code review

Co-authored-by: Lukas Pielsticker <[email protected]>

* add_NX_defs_for_beam_path

* modifying_yaml_files

* fixing_nyaml_make_file

* Adjusted files with Sandor together according to
earlier hardcoded .nxs file

* Added the missing nxdl.xml files via nyaml2nxdl
Version=0.0.8 was used for nyaml.

* moved created nxdl.xml files to correct directory

* Suggestions to fix ci/cd by in NXtransfer_matrix_table.yaml

Co-authored-by: Florian Dobener <[email protected]>

* renaming transfer_matrix_table to beam_transfermatrix_table and opt_element to beam_device; also merging NXopt_beam to NXbeam

* remove old nxdl files

---------

Co-authored-by: Ron Hildebrandt <[email protected]>
Co-authored-by: Lukas Pielsticker <[email protected]>
Co-authored-by: Florian Dobener <[email protected]>
# Conflicts:
#	base_classes/nyaml/NXbeam.yaml
@lukaspie lukaspie force-pushed the fairmat-2024-nxbeam branch from 5eb2ceb to 15cd321 Compare January 17, 2025 13:39
@lukaspie
Copy link
Contributor Author

Proposal from Telco:

  • Remove previous/next device from this PR
  • Remove NXbeam_element from this PR

Done here, should be ready for trying another vote.

  • Add inputs and outputs to NXcomponent: lists of other components (NX_CHAR, rank of 0 or 1 (single or multiple), but it's optional)

Implies that NXcomponent would include anything that can affect the beam

Implemented like this in #1525

A charge to the committee: go review and comment on #1525!! (consider implications of #1507)

Would want a flow/class diagram showing this inheritance. Also, don't want to lose the cool figure above of the multiple beam paths!

What's missing here is where to put this figure. Should the beam still have next/previous element? Or is this chaining just through the components?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NIAC vote needed PR needs an approving vote from NIAC before merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NXbeam
10 participants