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

Remove as_grid_scan from dials.import #2452

Open
graeme-winter opened this issue Jul 5, 2023 · 4 comments · May be fixed by #2454
Open

Remove as_grid_scan from dials.import #2452

graeme-winter opened this issue Jul 5, 2023 · 4 comments · May be fixed by #2454

Comments

@graeme-winter
Copy link
Contributor

As seen in #2447 this does nothing useful and is misleading - either

  • remove as_grid_scan
  • modify it to be an alias which sets scan.oscillation=0,0

I think the second is superior, will look
@JunAishima JunAishima mentioned this issue Jul 5, 2023
Closed
@JunAishima
Copy link

having the name grid_scan in there implies that it could enable processing the data in a way that the images may be related to each other. this might be useful for real grid scans, but simply changing it to stills would make sense for us (with these datasets).

@graeme-winter
Copy link
Contributor Author

having the name grid_scan in there implies that it could enable processing the data in a way that the images may be related to each other. this might be useful for real grid scans, but simply changing it to stills would make sense for us (with these datasets).

Conceptually I completely agree, however we don't do that right now and you would think we do. That's an answer in favour of simply eliminating the option, and we can put it back one day when we do "something special" with grid scans over simply importing as stills.

@JunAishima
Copy link

having the name grid_scan in there implies that it could enable processing the data in a way that the images may be related to each other. this might be useful for real grid scans, but simply changing it to stills would make sense for us (with these datasets).

Conceptually I completely agree, however we don't do that right now and you would think we do. That's an answer in favour of simply eliminating the option, and we can put it back one day when we do "something special" with grid scans over simply importing as stills.

Sounds reasonable!

@graeme-winter
Copy link
Contributor Author

(by way of a narrative annotation, I am getting crazy déjà vu doing this)

graeme-winter added a commit that referenced this issue Jul 5, 2023
@graeme-winter
Remove as_grid_scan from dials.import
032879a 
Is not useful, and is misleading as it makes data into stills rather than
image sequence. If we want to reinstate it, this can be reverted and fixed
up appropriately.

Fixes #2452

Mentined in #2447
@graeme-winter graeme-winter linked a pull request Jul 5, 2023 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove as_grid_scan from dials.import dials/dials
2 participants
@graeme-winter @JunAishima