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

Application definition for XPS #249

Merged
merged 15 commits into from
Jul 4, 2024
Merged

Application definition for XPS #249

merged 15 commits into from
Jul 4, 2024

Conversation

lukaspie
Copy link
Collaborator

  • Extends the non-merged NXxps application definition from NXxps sub app-def #30.
    • Defines coordinate system
    • Different levels of requiredness compared to NXmpes.

I pulled this out of #170 because that one still needs a bit of discussion, but we can add the peak fitting to NXxps later.

@lukaspie lukaspie linked an issue Jun 19, 2024 that may be closed by this pull request
5 tasks
@lukaspie lukaspie requested review from domna and rettigl June 19, 2024 11:00
Copy link
Collaborator

@rettigl rettigl left a comment

Choose a reason for hiding this comment

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

I think I don't understand all your transformations well, even not with the picture. I left some comments.

contributed_definitions/nyaml/NXxps.yaml Show resolved Hide resolved
contributed_definitions/nyaml/NXxps.yaml Show resolved Hide resolved
contributed_definitions/nyaml/NXxps.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXxps.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXxps.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXxps.yaml Show resolved Hide resolved
contributed_definitions/nyaml/NXxps.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXxps.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXxps.yaml Show resolved Hide resolved
contributed_definitions/xps/xps_cs.png Outdated Show resolved Hide resolved
@lukaspie
Copy link
Collaborator Author

@domna here is an example config and nxs file for just the transformations
vms_cs.zip

@domna
Copy link

domna commented Jun 27, 2024

@domna here is an example config and nxs file for just the transformations vms_cs.zip

Thank you for this file. I found two small problems:

  • I'm not sure of relative notation is supported if the fields are not in the same transformations group. At least the nexus3d tool couldn't follow it, but it's an easy fix
  • There is a small error in entry/instrument/beam_xray. I think the depends_on must be transformations/beam_azimuth_angle and not transformations/source_azimuth_angle

Copy link

@domna domna left a comment

Choose a reason for hiding this comment

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

I only commented on one transformation but the other ones look similar and I think the same changes should be applied there.

contributed_definitions/nyaml/NXxps.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXxps.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXxps.yaml Outdated Show resolved Hide resolved
@lukaspie
Copy link
Collaborator Author

@domna here is an example config and nxs file for just the transformations vms_cs.zip

Thank you for this file. I found two small problems:

  • I'm not sure of relative notation is supported if the fields are not in the same transformations group. At least the nexus3d tool couldn't follow it, but it's an easy fix

Where do you see this relative notation? You mean "depends_on": "transformations/beam_azimuth_angle" should be the full path?

  • There is a small error in entry/instrument/beam_xray. I think the depends_on must be transformations/beam_azimuth_angle and not transformations/source_azimuth_angle

Fixed (it is in the config on the reader side), you were right.

@lukaspie
Copy link
Collaborator Author

@domna this should bre ready for review. Here is the updated example:
vms_cs_updated.zip

@domna
Copy link

domna commented Jun 28, 2024

@domna this should bre ready for review. Here is the updated example: vms_cs_updated.zip

The electronanalyser lies at the same position as the beam in the example. Also the same values are read for both (e.g., @xps_token:beam_xray/source_polar_angle for the polar angle). Is this a typo?

@lukaspie
Copy link
Collaborator Author

lukaspie commented Jul 4, 2024

@domna I think we can merge this now and then refactor it once we have a proper description of transformations in #235

@lukaspie lukaspie merged commit 17399f1 into fairmat Jul 4, 2024
6 checks passed
@lukaspie lukaspie deleted the 246-finalize-nxxps branch July 4, 2024 14:51
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.

Finalize NXxps
3 participants