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

Add motor info to QNexuswidget #1088

Merged
merged 6 commits into from
Nov 27, 2024
Merged

Add motor info to QNexuswidget #1088

merged 6 commits into from
Nov 27, 2024

Conversation

mspito
Copy link
Contributor

@mspito mspito commented Aug 14, 2024

Needs #1087 first

Screenshot from 2024-08-14 16-15-52

@vasole
Copy link
Owner

vasole commented Aug 14, 2024

Please rebase your work on current master that includes #1087

@woutdenolf woutdenolf marked this pull request as ready for review August 14, 2024 16:23
@vasole
Copy link
Owner

vasole commented Aug 14, 2024

Some comments:

PyMca is used at other facilities than the ESRF and former ESRF files did not look like current ones.
I find nxpositioners = nxentry.get("instrument/positioners_start", None) very ESRF-centric and restrictive.

I would aim at using something similar to the function getPositionersGroup(h5file, path) in PyMca5.PyMcaCore.NexusTools.py That would make the use of the new feature not restricted to ESRF. For instance, A getStartingPositionersGroup in PyMca5.PyMcaCore.NexusTools.py seems to me more appropriate. It could use your hard-coded specific call and default to getPositionersGroup if nothing found. That would imply that if one positioner is an array of positions, the first position is used as starting point to allow the use of get_motor_positions.

@mspito
Copy link
Contributor Author

mspito commented Oct 14, 2024

I tried to address your suggestions. Is there anything else to be done?

src/PyMca5/PyMcaCore/NexusTools.py Outdated Show resolved Hide resolved
@vasole
Copy link
Owner

vasole commented Nov 18, 2024

I tried to address your suggestions. Is there anything else to be done?

Yes. It continues to be ESRF centric and only applicable to files generated recently.

Please take a look at NexusTools.getPositionersGroup

There you will find a more generic approach to figure out the positioners associated to a scan. In absence of the new ESRF positioners_start group, the information provided in that group is perfectly usable taking the first value of a positioner in case multiple values are present.

@vasole
Copy link
Owner

vasole commented Nov 18, 2024

The proposed feature should be of use on old and new ESRF files as well as Sardana HDF5 files.

@woutdenolf
Copy link
Collaborator

What makes you think positioners_start is "new"? As far as I can remember it has always been there.

@vasole
Copy link
Owner

vasole commented Nov 18, 2024

Prior to it there was a group inside NXinstrument named positioners and no positioners_start

@vasole
Copy link
Owner

vasole commented Nov 18, 2024

What makes you think positioners_start is "new"? As far as I can remember it has always been there.

If it serves as reminder, silx convert is still generating the NXinstrument/positioners group.

@woutdenolf
Copy link
Collaborator

For reference: NXinstrument/positioners provides motor positions in the scope of entire scan. This means static motors are scalars and moving motors are arrays. NXinstrument/positioners_start and NXinstrument/positioners_end are snapshots of motor positions at the start/end of the scan and are therefore scalars.

@vasole
Copy link
Owner

vasole commented Nov 18, 2024

For reference: NXinstrument/positioners provides motor positions in the scope of entire scan. This means static motors are scalars and moving motors are arrays. NXinstrument/positioners_start and NXinstrument/positioners_end are snapshots of motor positions at the start/end of the scan and are therefore scalars.

positioners is always there while start and end are not always there . Just convert a SPEC file with silx convert to verify it. Therefore, I want to check first for positioners_start, and if not present, to use positioners, and if not present to try the Sardana equivalents. It is a 10 minutes work. Simply I do not consider to be my job to do it.

@woutdenolf
Copy link
Collaborator

woutdenolf commented Nov 18, 2024

So what needs to be done is

  • remove getStartingPositionersGroup and use getPositionersGroup in get_motor_positions instead, taking into account the datasets can be 0D or 1D.
  • modify getPositionersGroup to look for positioners_start before looking for positioners.

Is that correct?

@vasole
Copy link
Owner

vasole commented Nov 18, 2024

One cannot escape to having two different functions (or a function with keywords).

You need to keep into account that to match the SPEC behaviour, you want the motor values associated to the positions when the command was issued. If the information is not available, then one uses the group provided by getPositionersGroup

What you propose should work for this issue, but when I look for the positioners group in PyMca, I actually want the group containing the scanned ones because I want the positions during the scan and not when the command was issued.

So, I would just extend the getStartingPositionersGroup to handle the case positioners_start is not there and it gets the getPositionersGroup instead. The handling of 1D data instead of 0D would indeed be necessary. If you want to centralize the dimensions handling as getStartingPositionersAndValues or getStartingPositioners with a keyword to get the values, or whatever, I am pretty sure it can avoid duplicating code somewhere else.

@mspito
Copy link
Contributor Author

mspito commented Nov 26, 2024

We added the function getStartingPositionerValues which calls the function getStartingPositionersGroup which falls back to the function getPositionersGroup. Is that what you had in mind to be more generic ?

@vasole
Copy link
Owner

vasole commented Nov 26, 2024

We added the function getStartingPositionerValues which calls the function getStartingPositionersGroup which falls back to the function getPositionersGroup. Is that what you had in mind to be more generic ?

Yes, it is. Thank you. Please, just make the new functionality is available only at NXentry level.

@woutdenolf
Copy link
Collaborator

We thought it would be easier if you could click anywhere in the scan and see the list of motors.

@vasole
Copy link
Owner

vasole commented Nov 26, 2024

I understand your point of view, but I always keep in mind PyMca is used to deal with HDF5 files from many sources (and not even Nexus files). My point of view is that in many cases the motors information will not be there.

To have systematically a Motors tab, associated to any group or dataset of an HDF5 file is overkill to me.

@mspito
Copy link
Contributor Author

mspito commented Nov 26, 2024

Now we can access to the motor positions only at NXentry level. Is that what you expected ?

@vasole vasole merged commit cb0b621 into vasole:master Nov 27, 2024
1 check passed
@vasole
Copy link
Owner

vasole commented Nov 27, 2024

Thanks a lot. I just added the license header to the new file prior to the merge.

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.

3 participants