Flux changes for NXmx #1035

phyy-nx · 2022-03-15T01:26:29Z

Add flux_integrated and total_flux_integrated, Useful where temporal profiles of flux are not known.
Change total_flux to not be required
Add NXbeam attribute flux, to point to which flux field is used
Add optional NXmonitor for additional incident beam data

- Add flux_integrated and total_flux_integrated, Useful where temporal profiles of flux are not known. - Change total_flux to not be required - Add NXbeam attribute flux, to point to which flux field is used - Add optional NXmonitor for additional incident beam data Closes #986

yayahjb

Good fix -- Herbert

applications/NXmx.nxdl.xml

woutdenolf · 2022-03-15T16:16:49Z

Referenced units:

NX_FLUX: 1/s/cm^2
NX_PER_AREA: 1/cm^2
NX_FREQUENCY: 1/s
NX_DIMENSIONLESS

This is what currently exists:

NXbeam
  flux[i]: (optional) NX_FLOAT {units=NX_FLUX}

NXmx
ENTRY: (required) NXentry
  BEAM: (required) NXbeam
   flux: (optional) NX_FLOAT {units=NX_FLUX}
   total_flux: (required) NX_FLOAT {units=NX_FREQUENCY}

This is the proposed changes in this MR (without the NXmonitor part, only the flux related things):

NXbeam
  flux[i]: (optional) NX_FLOAT {units=NX_FLUX}

NXmx
ENTRY: (required) NXentry
  BEAM: (required) NXbeam
   @flux: (optional) NX_CHAR
   total_flux: (optional) NX_FLOAT {units=NX_FREQUENCY}
   flux: (optional) NX_FLOAT {units=NX_FLUX}
   flux_integrated: (optional) NX_FLOAT {units=NX_PER_AREA}
   total_flux_integrated: (optional) NX_FLOAT {units=NX_DIMENSIONLESS}

So the word "integrated" stands for time integrated and the word "total" stands for area integrated.

The reason we cannot come up with sensible names like

flux_integrated -> flux_time_integrated
total_flux -> flux_area_integrated
total_flux_integrated -> flux_time_and_area_integrated

is because NXmx already has total_flux.

Sorry @phyy-nx for my initial pessimistic view, the two units that were wrong scared the hell out of me. The names are very unfortunate but we already have total_flux in NXmx so I don't see another way either. I'd say we should not propagate these unfortunate names to NXbeam in the future.

I have no strong opinion on the addition of the optional NXmonitor.

phyy-nx · 2022-03-15T18:32:54Z

No worries, thanks for the comments. I'll review your comments more carefully as soon as I get a chance.

yayahjb · 2022-03-15T18:53:43Z

All existing MX ontologies have some unfortunate names, but it can be a disaster to make changes to existing tags used in large numbers of images, in this case, millions. The best you can do it to start a slow process of deprecation and recommendation of new names. Most of the tme it is not worth the effort. It could be worse. We could be back to when I was young and all of physics moved from default left-handed systems for E&M to right handed. I still have trouble with that, especially since I am a right-trained lefty.

…

On Tue, Mar 15, 2022 at 12:17 PM woutdenolf ***@***.***> wrote: Referenced units: - NX_FLUX: 1/s/cm^2 - NX_PER_AREA: 1/cm^2 - NX_FREQUENCY: 1/s - NX_DIMENSIONLESS This is what currently exists: NXbeam flux[i]: (optional) NX_FLOAT {units=NX_FLUX} NXmx ENTRY: (required) NXentry BEAM: (required) NXbeam flux: (optional) NX_FLOAT {units=NX_FLUX} total_flux: (required) NX_FLOAT {units=NX_FREQUENCY} This is the new proposal (without the NXmonitor part, only the *flux* related things): NXbeam flux[i]: (optional) NX_FLOAT {units=NX_FLUX} NXmx ENTRY: (required) NXentry BEAM: (required) NXbeam @Flux: (optional) NX_CHAR total_flux: (optional) NX_FLOAT {units=NX_FREQUENCY} flux: (optional) NX_FLOAT {units=NX_FLUX} flux_integrated: (optional) NX_FLOAT {units=NX_PER_AREA} total_flux_integrated: (optional) NX_FLOAT {units=NX_DIMENSIONLESS} So the word "integrated" stands for *time integrated* and the word "total" stands for *area integrated*. The reason we cannot come up with sensible names like - flux_integrated -> flux_time_integrated - total_flux -> flux_area_integrated - total_flux_integrated -> flux_time_and_area_integrated is because NXmx already has total_flux. Sorry @phyy-nx <https://github.com/phyy-nx> for my initial pessimistic view, the two units that were wrong scared the hell out of me. The names are very unfortunate but we already have total_flux in NXmx so I don't see another way either. I'd say we should not propagate these unfortunate names to NXbeam in the future. I have no strong opinion on the addition of the optional NXmonitor. — Reply to this email directly, view it on GitHub <#1035 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABB6EAKFIQRHI7ZFJ3YFTNDVACZXXANCNFSM5QXGUGGA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because your review was requested.Message ID: ***@***.***>

woutdenolf · 2022-03-15T19:24:26Z

I'll review your comments more carefully as soon as I get a chance.

@phyy-nx There is nothing to review, I didn't propose anything different. You can go ahead with what you have after fixing the units of flux_integrated and total_flux_integrated. I just described in my own words what my understanding is of your changes. If this corresponds with what you intended, then I have no further comments. After fixing those two units it is all clear now. The names are unfortunate but as I said, I don't think we can do better provided what is already in the standard.

but it can be a disaster to make changes to existing tags

For sure.

woutdenolf · 2022-03-15T19:35:55Z

@phyy-nx Maybe you could add a comment about flux integration to the respective fields?

flux_integrated: flux integrated over time
total_flux: flux integrated over area
total_flux_integrated: flux integrated over time and area

You can understand that from the units but maybe it is good to mention it explicitly since the words "total" and "integrated" are not self explanatory.

prjemian · 2022-06-02T17:14:57Z

@phyy-nx Since this is labeled "NIAC should review", then is there anything to do at the Code Camp this month? If not, then remove this PR from the Code Camp project.

woutdenolf · 2022-06-14T11:44:14Z

I proposed a fix for all my comments in Merge #1082. You can merge it in this branch if you approve.

(whitespace change too)

phyy-nx · 2022-06-14T17:05:53Z

Ok, I think that's all the feedback from the spring NIAC. I think we can remove the code camp label and vote on it this fall.

woutdenolf

LGTM

prjemian · 2022-06-14T19:57:51Z

GitHub reports a merge conflict with NXmx.nxdl.xml.

Also, since we will not merge this until the NIAC looks at it, can we convert this PR to draft state to prevent someone merging?

phyy-nx · 2022-06-14T21:28:51Z

Conflicts are fixed

phyy-nx · 2022-06-16T21:24:02Z

Conversation from code camp summer 2022. Since this needs a NIAC vote anyway, let's make as a goal to use the logic proposed in #1002 to enforce a choice between flux, total_flux, flux_integrated, total_flux_integrated, and the path to an NXmonitor with per-shot flux information. We'd like to have that ready by the fall NIAC meeting. Owners: @benajamin for finishing the new NXDL rule needed to enforce choices between fields and groups, and @phyy-nx to add the logic to this PR.

phyy-nx · 2022-09-14T16:50:02Z

From Fall 2022 NIAC, don't need to wait on #1002. Instead, vote on this, merge it, and add a new issue to require one of flux, total_flux, flux_integrated, total_flux_integrated

prjemian · 2022-09-14T16:58:29Z

There is a conflict involving the NXmonitor group.

phyy-nx · 2022-09-14T22:15:45Z

Conflicts fixed, ready for NIAC vote

applications/NXmx.nxdl.xml

@prjemian

thanks @prjemian

prjemian · 2022-09-15T14:17:50Z

CI testing failed:

Warning, treated as error:
/home/runner/work/definitions/definitions/build/manual/source/classes/applications/NXmx.rst:1005:undefined label: 'nxmx-entry-instrument-beam-total-flux-field'
make: *** [Makefile:62: html] Error 2
Error: Process completed with exit code 2.

And that means my previous change was not correct. My apology. Please switch back to:

:ref:`flux &lt;/NXmx/ENTRY/INSTRUMENT/BEAM/flux-field&gt;`

…te that the user can provide a link to an NXmonitor

This reverts commit 85b5288.

prjemian

LGTM

benajamin · 2022-09-15T15:05:03Z

The proposal is to accept the changes to NXmx represented by pull request #1035

phyy-nx · 2022-09-15T15:39:05Z

Thanks folks

phyy-nx added the NIAC should review The NIAC should review/discuss label Mar 15, 2022

phyy-nx requested review from yayahjb March 15, 2022 02:32

yayahjb approved these changes Mar 15, 2022

View reviewed changes

phyy-nx commented Mar 15, 2022

View reviewed changes

applications/NXmx.nxdl.xml Outdated Show resolved Hide resolved

phyy-nx commented Mar 15, 2022

View reviewed changes

applications/NXmx.nxdl.xml Outdated Show resolved Hide resolved

phyy-nx commented Mar 15, 2022

View reviewed changes

applications/NXmx.nxdl.xml Outdated Show resolved Hide resolved

phyy-nx commented Mar 15, 2022

View reviewed changes

applications/NXmx.nxdl.xml Outdated Show resolved Hide resolved

phyy-nx commented Mar 15, 2022

View reviewed changes

applications/NXmx.nxdl.xml Outdated Show resolved Hide resolved

prjemian assigned phyy-nx Jun 2, 2022

prjemian added this to the NXDL 2022.06 milestone Jun 14, 2022

prjemian added the code camp label Jun 14, 2022

woutdenolf and others added 3 commits June 14, 2022 09:53

fix flux units and improve docs

2732d34

Take out maxOccurs=1 for NXmonitor

4b432ee

Add note that the flux attribute can point to an NXmonitor group

08fa6a9

(whitespace change too)

phyy-nx removed the code camp label Jun 14, 2022

woutdenolf self-requested a review June 14, 2022 17:19

woutdenolf approved these changes Jun 14, 2022

View reviewed changes

phyy-nx marked this pull request as draft June 14, 2022 21:09

Merge branch 'main' into nxmx_flux

a42f0ca

benajamin mentioned this pull request Jun 17, 2022

NXmx total_flux nexusformat/NIAC#96

Closed

prjemian modified the milestones: NXDL 2022.06, NXDL 2023.06 Jun 24, 2022

Merge branch 'main' into nxmx_flux

a40bdc6

phyy-nx marked this pull request as ready for review September 14, 2022 22:15

yayahjb approved these changes Sep 15, 2022

View reviewed changes

prjemian reviewed Sep 15, 2022

View reviewed changes

applications/NXmx.nxdl.xml Show resolved Hide resolved

prjemian reviewed Sep 15, 2022

View reviewed changes

applications/NXmx.nxdl.xml Show resolved Hide resolved

benajamin reviewed Sep 15, 2022

View reviewed changes

applications/NXmx.nxdl.xml Outdated Show resolved Hide resolved

Apply code review

85b5288

thanks @prjemian

phyy-nx added 2 commits September 15, 2022 07:28

Remove the optional NXmonitor and reword the flux attribute to indica…

b2c7e96

…te that the user can provide a link to an NXmonitor

Revert "Apply code review"

34e9436

This reverts commit 85b5288.

prjemian approved these changes Sep 15, 2022

View reviewed changes

benajamin added NIAC approved and removed NIAC should review The NIAC should review/discuss labels Sep 15, 2022

phyy-nx merged commit e3f64e9 into main Sep 15, 2022

phyy-nx deleted the nxmx_flux branch September 15, 2022 15:39

benajamin mentioned this pull request Jan 5, 2023

Can we have a standard rule in NeXus to validate presence of one item from a list of possibles? #1002

Open

PeterC-DLS modified the milestones: NXDL 2023.06, NXDL 2023.10 Jan 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flux changes for NXmx #1035

Flux changes for NXmx #1035

phyy-nx commented Mar 15, 2022 •

edited

Loading

yayahjb left a comment

woutdenolf commented Mar 15, 2022 •

edited

Loading

phyy-nx commented Mar 15, 2022

yayahjb commented Mar 15, 2022 via email

woutdenolf commented Mar 15, 2022 •

edited

Loading

woutdenolf commented Mar 15, 2022

prjemian commented Jun 2, 2022

woutdenolf commented Jun 14, 2022

phyy-nx commented Jun 14, 2022

woutdenolf left a comment

prjemian commented Jun 14, 2022

phyy-nx commented Jun 14, 2022

phyy-nx commented Jun 16, 2022

phyy-nx commented Sep 14, 2022

prjemian commented Sep 14, 2022

phyy-nx commented Sep 14, 2022

prjemian commented Sep 15, 2022 •

edited

Loading

prjemian left a comment

benajamin commented Sep 15, 2022

phyy-nx commented Sep 15, 2022

Flux changes for NXmx #1035

Flux changes for NXmx #1035

Conversation

phyy-nx commented Mar 15, 2022 • edited Loading

yayahjb left a comment

Choose a reason for hiding this comment

woutdenolf commented Mar 15, 2022 • edited Loading

phyy-nx commented Mar 15, 2022

yayahjb commented Mar 15, 2022 via email

woutdenolf commented Mar 15, 2022 • edited Loading

woutdenolf commented Mar 15, 2022

prjemian commented Jun 2, 2022

woutdenolf commented Jun 14, 2022

phyy-nx commented Jun 14, 2022

woutdenolf left a comment

Choose a reason for hiding this comment

prjemian commented Jun 14, 2022

phyy-nx commented Jun 14, 2022

phyy-nx commented Jun 16, 2022

phyy-nx commented Sep 14, 2022

prjemian commented Sep 14, 2022

phyy-nx commented Sep 14, 2022

prjemian commented Sep 15, 2022 • edited Loading

prjemian left a comment

Choose a reason for hiding this comment

benajamin commented Sep 15, 2022

phyy-nx commented Sep 15, 2022

phyy-nx commented Mar 15, 2022 •

edited

Loading

woutdenolf commented Mar 15, 2022 •

edited

Loading

woutdenolf commented Mar 15, 2022 •

edited

Loading

prjemian commented Sep 15, 2022 •

edited

Loading