-
Notifications
You must be signed in to change notification settings - Fork 56
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
base: main
Are you sure you want to change the base?
Conversation
2c737d2
to
5b7cc87
Compare
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. |
There was a problem hiding this 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.
2529db0
to
c75d5c2
Compare
@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. |
c75d5c2
to
570a7e8
Compare
Comments from Telco:
Once done, based on Telco conversation, we can put this up for an online vote |
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:
As discussed on the call, these two fields allow connecting devices together. A figure was shown demonstrating the usage. We decided that the Any questions we can answer here. Otherwise, cast a vote! Thanks! |
Can the previous/next thing be an |
My thought was that this is for components hooked together. The only usage of @sanbrock @mkuehbach @lukaspie is connecting a beam to a sample directly a use case you are interested in? |
There was a problem hiding this 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)
Indeed, we mainly focused on hooking together components. But, having an I think we would be open to remaining to something more general like Also tagging in @RonHildebrandt here. |
I apologize for missing the telco and possibly asking redundant questions:
|
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?
This is indeed something that was not really considered so far. I see two ways to cover this:
I think option 1) would be the cleanest solution. |
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? |
13 votes isn't enough to pass, which is fine due to the continuing discussion. We'll review in the next Telco. |
base_classes/NXbeam.nxdl.xml
Outdated
This defines whether the beam is an "input" or "output" beam. | ||
</doc> | ||
</field> | ||
<field name="next_device"> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
da81fb9
to
3599458
Compare
Proposal from Telco:
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! |
… 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
Co-authored-by: RubelMozumder <[email protected]>
5eb2ceb
to
15cd321
Compare
Done here, should be ready for trying another vote.
Implemented like this in #1525
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? |
With this PR, the idea is:
incident_energy
field inNXbeam
is to be used. Specfically, for polychromatic beams, a single fieldincident_energy
is not sufficient, but rather one needs information about theincident_energy_spread
. In addition, for a polychromatic beam, it is useful to incidate theincident_energy_weights
, i.e., how much each each of energies contributes to the overall intensity.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).