-
Notifications
You must be signed in to change notification settings - Fork 9
Add support for defining scans using scanspec #40
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 support for defining scans using scanspec #40
Conversation
|
This pull request introduces 1 alert when merging 629ad0a into 5a0dbb7 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging f36c685 into 018f7f1 - view on LGTM.com new alerts:
|
|
Unfortunately |
|
After discussion with @noemifrisina it was decided we could remove 3.6 support. Have made a ticket to do so at #44 |
| ], | ||
| ), | ||
| ) | ||
| if len(scan_range.keys()) == 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick note before I forget, for the moment I would leave this as it was, as in our case "axes" and "axes_indices" point to the first dimension of the "data" dataset - which pretty much amounts to the frame number, and the "rotation" axis. Even if that is not moving...
(this is part of the "making sure everything else doesn't break in the process" thing I was talking about yesterday)
I'd leave thinking what to do with these attributes for when we have the grid scans running - and I have another look at what happens in processing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think for what I am currently passing in from artemis if I left it at what this was the scan axis will be set to one of "sam_x" or "sam_y". We actually want it set to omega, right? I'm not sure if the logic for adding the omega in should be done in artemis or nexgen. This will actually be solved in when we move to the 3D gridscan as Artemis will then need to provide omega
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for the moment it should stay set on the rotation axis (which could be omega or phi depending on the beamline). Honestly, I'd rather the logic for this stays in nexgen, because artemis is not the only project that calls these functions and I need it to work for other applications too... artemis will provide omega for the 3D gridscans of course, but if for example I'm writing for events on I19 I do need this to still write the correct attributes
It looks like I'll probably need to make some changes to how the rotation axis is saved anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it should be in nexgen. Maybe we should have a chat about what the best interface looks like for setting scan info then as we need top provide it in such a way that nexgen knows it's a gridscan and to create the chi/phi part itself.
37f48a4 to
ffb7917
Compare
|
Closing in favour of: |
Adds support for defining the scan axes as scanspecs instead of having to create numpy arrays from scratch. This makes it easier to write arbitrary scan rather than just rotations.
The biggest change to existing code was to replace
scan_rangewith a dictionary that could have many axes rather than a numpy array of the points for just one axis. This hopefully hasn't broken any pre-existing code.Note there are some places in the code where some further discussion of how to handle them with multiple axes would be good.
I have also added an example file for how a gridscan might be created and some tests for any changes I have made.