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.

Sign up for GitHub

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

Jump to bottom

Plan for SPM app def. #241

Open
wants to merge 15 commits into
base: fairmat
Choose a base branch
from
Open

Plan for SPM app def. #241

wants to merge 15 commits into from

Conversation

RubelMozumder
Copy link

@RubelMozumder RubelMozumder commented Jun 17, 2024 •
edited
Loading

  • Modify sample NXhistory for NXsample.
  • Use NX_CURRENT_OVER_VOLTAGE or NX_ANY where is A/V is being used.
  • Try to think to use NXscan_control to represent data NXbias_spectroscopy
  • Remove SPM folder before the final marge.
  • Remove the unnecessary files.

@RubelMozumder RubelMozumder mentioned this pull request Jun 18, 2024
Merged
lukaspie
lukaspie reviewed Jun 25, 2024
View reviewed changes
Copy link
Collaborator

@lukaspie lukaspie left a comment

Choose a reason for hiding this comment

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

These are the things I saw immediately when going through the file, there will probably be more review in the future.

contributed_definitions/nyaml/NXscan_control.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXscan_control.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXscan_control.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXscan_control.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXscan_control.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXscan_control.yaml Show resolved Hide resolved
contributed_definitions/nyaml/NXscan_control.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXscan_control.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXscan_control.yaml Outdated Show resolved Hide resolved
@RubelMozumder
Copy link
Author

These are the things I saw immediately when going through the file, there will probably be more review in the future.

Thanks for your spontaneous review. I will take care of them.

@RubelMozumder
Copy link
Author

@lukaspie can you go through the other files in this PR. Mainly, I would suggest to check the structure and concept such as units, dimensionality and so on. Still some of the text from old version so sometime they are explained not correctly.

lukaspie
lukaspie requested changes Jun 26, 2024
View reviewed changes
Copy link
Collaborator

@lukaspie lukaspie left a comment

Choose a reason for hiding this comment

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

Adding more comments on the other files. Mostly wording, some question about certain concepts, and checks for units/dimensions.

contributed_definitions/nyaml/NXbias_spectroscopy.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXbias_spectroscopy.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXbias_spectroscopy.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXbias_spectroscopy.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXbias_spectroscopy.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXspm.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXspm.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXspm.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXspm.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXspm.yaml Outdated Show resolved Hide resolved
@RubelMozumder
Copy link
Author

RubelMozumder commented Jun 26, 2024 •
edited
Loading

@lukaspie I am leaving all the change requests for NXspm resolved. This is not the part of this scrum, for now we are considering only base classes. But, definitely when I use the base classes in the NXspm and modify that we need a full review on it.

Note: I have also restructure NXbias_spectroscopy, probably you did not find some of the fields. Just not to be confused.

@lukaspie
Copy link
Collaborator

@lukaspie I am leaving all the change requests for NXspm resolved. This is not the part of this scrum, for now we are considering only base classes. But, definitely when I use the base classes in the NXspm and modify that we need a full review on it.

I don't understand this. Why is NXspm changed here at all then? If it's not to be addressed here, I would suggest to remove any changes on NXspm in this PR. Otherwise, at least leave a comment in the yaml file so that these comments here don't get lost.

RubelMozumder
RubelMozumder commented Jun 27, 2024
View reviewed changes
base_classes/nyaml/NXpositioner.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXfabrication.yaml Show resolved Hide resolved
contributed_definitions/nyaml/NXsts.yaml Outdated Show resolved Hide resolved
base_classes/nyaml/NXpositioner.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXscan_control.yaml Show resolved Hide resolved
contributed_definitions/nyaml/NXpiezo_config_spm.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXpiezo_config_spm.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXpiezo_config_spm.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXpiezo_config_spm.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXpiezo_config_spm.yaml Outdated Show resolved Hide resolved
lukaspie
lukaspie approved these changes Jun 27, 2024
View reviewed changes
Copy link
Collaborator

@lukaspie lukaspie left a comment

Choose a reason for hiding this comment

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

LGTM (please fix the one remaining issue). NXspm needs to be heavily refactored in the future, as it is now an appdef extending the base class NXsensor_scan.

@RubelMozumder
Copy link
Author

SPM application definition for (STM, STS and AFM) is being handled here #259

@RubelMozumder RubelMozumder force-pushed the SPM_domain_app_def branch 2 times, most recently from af9a5c9 to d2d9d83 Compare October 21, 2024 10:25
GinzburgLev
GinzburgLev reviewed Oct 31, 2024
View reviewed changes
Copy link

@GinzburgLev GinzburgLev left a comment

Choose a reason for hiding this comment

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

Some comments now, will continue later

contributed_definitions/nyaml/SPM/NXafm.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXafm.yaml Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXbias_spectroscopy.yaml Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXbias_spectroscopy.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXbias_spectroscopy.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXscan_control.yaml Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXscan_control.yaml Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXscan_control.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXscan_control.yaml Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXscan_control.yaml Show resolved Hide resolved
GinzburgLev
GinzburgLev reviewed Oct 31, 2024
View reviewed changes
Copy link

@GinzburgLev GinzburgLev left a comment

Choose a reason for hiding this comment

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

A next batch of comments, those are for NXbias_sweep, NXcantilever_spm and NXlockin. More will arrive later.

contributed_definitions/nyaml/SPM/NXbias_sweep.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXbias_sweep.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXbias_sweep.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXbias_sweep.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXbias_sweep.yaml Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXlockin.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXlockin.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXlockin.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXlockin.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXlockin.yaml Outdated Show resolved Hide resolved
GinzburgLev
GinzburgLev reviewed Nov 6, 2024
View reviewed changes
Copy link

@GinzburgLev GinzburgLev left a comment

Choose a reason for hiding this comment

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

More comments, these are on NXpiezo_config, NXpiezoelectric_material, NXpositioner_spm, NXrcs, NXsensor_scan and NXspm

contributed_definitions/nyaml/SPM/NXpiezoelectric_material.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXpiezoelectric_material.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXpiezoelectric_material.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXpiezoelectric_material.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXpiezo_config_spm.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXspm.yaml Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXspm.yaml Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXspm.yaml Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXspm.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXspm.yaml Outdated Show resolved Hide resolved
@RubelMozumder RubelMozumder marked this pull request as ready for review November 7, 2024 07:40
GinzburgLev
GinzburgLev reviewed Nov 7, 2024
View reviewed changes
contributed_definitions/nyaml/SPM/NXsts.yaml Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXstm.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXstm.yaml Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXstm.yaml Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXstm.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXstm.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXstm.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXstm.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/SPM/NXstm.yaml Show resolved Hide resolved
@RubelMozumder RubelMozumder force-pushed the SPM_domain_app_def branch 2 times, most recently from 21e585e to 378151f Compare November 8, 2024 06:03
lukaspie
lukaspie reviewed Nov 11, 2024
View reviewed changes
Copy link
Collaborator

@lukaspie lukaspie left a comment

Choose a reason for hiding this comment

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

Mostly focussing on being in line with the NIAC modifications.

base_classes/NXpositioner.nxdl.xml Show resolved Hide resolved
contributed_definitions/nyaml/NXafm.yaml Show resolved Hide resolved
contributed_definitions/nyaml/NXafm.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXcalibration.yaml Show resolved Hide resolved
contributed_definitions/nyaml/NXcalibration.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXscan_control.yaml Show resolved Hide resolved
contributed_definitions/nyaml/NXscan_control.yaml Outdated Show resolved Hide resolved
contributed_definitions/nyaml/NXsensor_scan.yaml Show resolved Hide resolved
contributed_definitions/nyaml/NXspm.yaml Show resolved Hide resolved
contributed_definitions/nyaml/README.md Show resolved Hide resolved
@RubelMozumder RubelMozumder force-pushed the SPM_domain_app_def branch 2 times, most recently from f523e4c to bb9d652 Compare November 29, 2024 07:20
RubelMozumder and others added 11 commits November 29, 2024 08:47
@RubelMozumder
First final draft for application definiton NXsts,
dc8f486 
NXstm, NXaf, NXspm and base classes NXlockin,
NXbias_spectroscopy, cantilever_spm, NXbias_sweep,
NXpiezo_config_spm, NXpositioner_spm, NXscan_control,
NXpiezoelectric_material and NXrcs.

Finish review comments from Lev.
@RubelMozumder
Remove SPM folder.
668fa1e
@RubelMozumder
Revert "Remove SPM folder."
7685400 
This reverts commit 3aeb39e.
@RubelMozumder
PR review from Lukas!
69d4b0a
@RubelMozumder
Delete unnecessary files.
cd69d81
@RubelMozumder
Revert "Delete unnecessary files."
c00e7c3 
This reverts commit 317a367.
@RubelMozumder
Include the default key attribute on entry and include NXhistory unde…
662b977 
…r the NXsample for verbose purpose.
@RubelMozumder @tomio13
Review from Tamash (#303)
daf00bf 
* space.

* Modified some descriptions trying to clarify the meaning of the elements. Please check them! Modified:Update NXafm.yaml

* NXbias_spectroscopy.yaml

* Update NXscan.yaml

I just added a slight clarification what we mean under the word scan.
However, it makes me wonder whether this should be more a base class than an application definition.
And whether the control parameter set should be only positional.
So, I have some doubts here...

* Update NXcantilever_spm.yaml

added some explanation and clarification to the cantilever properties and what are the driving details of cantilever oscillations

* Update NXcircuit.yaml

minor changes to allow for multichannel boards, e.g. 4 or 16 channel A/D converters or amplifiers

* Update NXlockin.yaml

Minor changes in documentation,
removed some redundant information on the demodulated signal.

* Update NXpiezo_config_spm.yaml

minimal changes in docstrings

* Update NXpositioner_spm.yaml

minor correction, the Z positioner has no idea where the sample is, so sample - probe distance is unknown at this point

* Update NXscan_control.yaml

some minor comment changes

* Generated nxdl files.

* Revert "Update NXscan.yaml"

This reverts commit 3f376f0.
This file is covered by #304  #304
---------

Co-authored-by: T. Haraszti <[email protected]>
@RubelMozumder
Rename lockin_current_flip_number to loockin_current_flip_sign.
741a990
@RubelMozumder
add NXidentifier in NXsensor_scan.
2a7775a
@RubelMozumder
Delete unnecessary file.
8c6b03c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GinzburgLev GinzburgLev GinzburgLev left review comments

@lukaspie lukaspie lukaspie approved these changes

@sanbrock sanbrock sanbrock left review comments

@tomio13 tomio13 tomio13 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RubelMozumder @lukaspie @tomio13 @sanbrock @GinzburgLev