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

Update appdef for nxsts #182

Merged
merged 16 commits into from
May 3, 2024
Merged

Update appdef for nxsts #182

merged 16 commits into from
May 3, 2024

Conversation

RubelMozumder
Copy link

@RubelMozumder RubelMozumder commented Feb 21, 2024

Bringing all the plots like single_points and line_scan on the same level and out of the NXdata group.

@RubelMozumder RubelMozumder changed the title Update appdef for n xsts Update appdef for nxsts Feb 23, 2024
Copy link

@sanbrock sanbrock left a comment

Choose a reason for hiding this comment

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

The only remaining point is the ambiguity of the too many options where and how 'version' and model name can be registered in NXfabrication.

@RubelMozumder
Copy link
Author

The only remaining point is the ambiguity of the too many options where and how 'version' and model name can be registered in NXfabrication.

@RubelMozumder
Copy link
Author

@sanbrock Sorry do not be confused seeing still old version of appdef. Trying the ci/cd why it is failed?

@RubelMozumder
Copy link
Author

@sanbrock I do not know why ci/cd failed. I have to check it. If you agree with the change please resolve the issue then I can start with the reader update respecting the new version of the PR.

@RubelMozumder
Copy link
Author

@mkuehbach, @domna, and @lukaspie, in the Fabrication I have extended mode field by a new version attribute. Mainly, each model there may have several versions such as 5e and 4.5 from generic model. Do you see any conflict with this little change with your existing application definition?

@lukaspie
Copy link
Collaborator

@mkuehbach, @domna, and @lukaspie, in the Fabrication I have extended mode field by a new version attribute. Mainly, each model there may have several versions such as 5e and 4.5 from generic model. Do you see any conflict with this little change with your existing application definition?

Should be fine from the MPES side.

@RubelMozumder
Copy link
Author

@mkuehbach, @domna, and @lukaspie, in the Fabrication I have extended mode field by a new version attribute. Mainly, each model there may have several versions such as 5e and 4.5 from generic model. Do you see any conflict with this little change with your existing application definition?

Should be fine from the MPES side.

Thanks for your quick reply.

Copy link
Collaborator

@mkuehbach mkuehbach left a comment

Choose a reason for hiding this comment

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

Hi @RubelMozumder,

The concept model is an arbitrary tech-partner specific brand name, I support that the docstring should be changed in that "when there is ambiguity about which model (version) we are talking one should write the model version in "model", I would not make an own attribute version here an example
Nion microscope Hermes 100 versus Hermes 200 I would write better the docstring such that people should users write either of these versions but not just "Hermes" into model, therefore use this docstring:
"Model of the component as it is named by the manufacturer.
If different versions exist are possible, the value in this field should be made specific enough to resolve the version."

Rest of the PR looks good to me

@RubelMozumder
Copy link
Author

Hi @RubelMozumder,

The concept model is an arbitrary tech-partner specific brand name, I support that the docstring should be changed in that "when there is ambiguity about which model (version) we are talking one should write the model version in "model", I would not make an own attribute version here an example Nion microscope Hermes 100 versus Hermes 200 I would write better the docstring such that people should users write either of these versions but not just "Hermes" into model, therefore use this docstring: "Model of the component as it is named by the manufacturer. If different versions exist are possible, the value in this field should be made specific enough to resolve the version."

Rest of the PR looks good to me

Done!

@RubelMozumder
Copy link
Author

@sanbrock, now it can be approved to be merged.

Copy link
Collaborator

@Souikey Souikey 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 for your modification. I didn't find any problem in these changes.

Copy link

@sanbrock sanbrock left a comment

Choose a reason for hiding this comment

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

LGTM

@sanbrock sanbrock dismissed mkuehbach’s stale review May 3, 2024 08:15

request is considered in the new commits

@sanbrock sanbrock merged commit 229774c into fairmat May 3, 2024
6 checks passed
@RubelMozumder RubelMozumder deleted the Update_appdef_for_NXsts branch May 16, 2024 09:03
lukaspie pushed a commit that referenced this pull request Sep 24, 2024
* Repositioning the several NXdata and including analytical_data NXdata.

* update NXsts.

* current_gain data type in NXsts and Name in NXfabrication

* ci/cd

* fix opt.

* fix opt.

* Doc for NXdata

* Fixing error coming from nexus parser in nomad.

* Version in NXfabrication.

* Resolve conundrum between name and version.

* regen NXsts.yaml.

* docs.

* update docs in NXfabrication.

* update docs in NXfabrication.

* Updating the NXenviroment docs.

* update NXenvironment docs.
# Conflicts:
#	contributed_definitions/NXlockin.nxdl.xml
#	contributed_definitions/NXsts.nxdl.xml
#	contributed_definitions/nyaml/NXfabrication.yaml
#	contributed_definitions/nyaml/NXlockin.yaml
#	contributed_definitions/nyaml/NXsts.yaml
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.

5 participants