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

dials.sweep_to_stills #917

Merged
merged 7 commits into from
Sep 12, 2019
Merged

dials.sweep_to_stills #917

merged 7 commits into from
Sep 12, 2019

Conversation

phyy-nx
Copy link
Member

@phyy-nx phyy-nx commented Sep 6, 2019

New program: dials.sweep_to_stills

Converts a sweep to a series of stills by using the goniometer and scan to create a stills Experiment for every scan point in the scan. Includes an oscillation_angle/2 offset to the A matrices to account for how scans are recorded in DIALS.

Also includes reflection splitting. Idea is that the indexed reflections from the sweep need to be split into slices so that each experiment gets all the spots that impinged on it, even if the centroid is on the next phi step.

Also also adds parameter check_reference_geometry=True, expert_level=2, to dials.import

phyy-nx and others added 2 commits September 6, 2019 12:33
….import

If False, skip the check reference detector step. Useful when the known headers are very wrong, such as the pixel size is off.
Converts a sweep to a series of stills by using the goniometer and scan to create a stills Experiment for every scan point in the scan. Includes an oscillation_angle/2 offset to the A matrices to account for how scans are recorded in DIALS.

Also includes reflection splitting. Idea is that the indexed reflections from the sweep need to be split into slices so that each experiment gets all the spots that impinged on it, even if the centroid is on the next phi step.

Co-authored-by: Kevin Dalton <kevinmdalton@gmail.com>
@dagewa
Copy link
Member

dagewa commented Sep 7, 2019

The functionality sounds useful. Please include tests in this repository for any new command line programs.

@ndevenish
Copy link
Member

I'm curious what specifically this is useful for?

Also, it'd be great if you could add a newsfragments/917.feature file with a short note saying this has been added and what it's for. (this is towncrier, which we're using to attempt to be better at building useful release notes)

@Anthchirp
Copy link
Member

Please see the program boilerplate for an updated template for new tools.
Although slightly out of date (#858) it is still more modern than the Script() helper class.

@dagewa
Copy link
Member

dagewa commented Sep 9, 2019

Some electron diffraction datasets (such as the one described in this paper) were collected as a series of discrete tilt steps on a single crystal. You could index and refine the whole dataset as a sweep, but would want to convert to stills for integration.

I imagine the LBL use case is something different though. It would be interesting to hear what that is.

Copy link
Contributor

@rjgildea rjgildea left a comment

Choose a reason for hiding this comment

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

As per other comments:

  • Please add a test for this new command line program
  • Please could you avoid using the outdated Script() way of writing command line programs

I've also added a few comments/suggestions inline with the changes.

command_line/sweep_to_stills.py Outdated Show resolved Hide resolved
command_line/sweep_to_stills.py Outdated Show resolved Hide resolved
command_line/sweep_to_stills.py Outdated Show resolved Hide resolved
command_line/sweep_to_stills.py Outdated Show resolved Hide resolved
command_line/sweep_to_stills.py Outdated Show resolved Hide resolved
command_line/sweep_to_stills.py Outdated Show resolved Hide resolved
command_line/sweep_to_stills.py Outdated Show resolved Hide resolved
command_line/sweep_to_stills.py Outdated Show resolved Hide resolved
command_line/sweep_to_stills.py Outdated Show resolved Hide resolved
@phyy-nx
Copy link
Member Author

phyy-nx commented Sep 9, 2019

Thanks all, I've addressed as many code review comments as I can for right now. Was particularly pleased about using set_A directly. Thanks @rjgildea. I don't have time to fix the boiler point stuff now, sorry.

I'll send more about the usecase particulars when I can. Appreciate the curiosity :)

@phyy-nx
Copy link
Member Author

phyy-nx commented Sep 12, 2019

(I'll merge tomorrow if that's ok)

- Split class __init__ and run() to more standard run (which loads the
  arguments from command line and writes the data back out) and
  sweep_to_stills (which transforms the data). This makes it easier to
  use programatically from code.
- Remove redundant set_dispatcher_name
- Remove shebang
- Use program name in help string directly
- use sys.exit so that exit code is set on bad input
- Use docstring instead of separate help_message
@ndevenish
Copy link
Member

I've updated to use the more modern layout. This touches a lot, because mainly it's just removing the class and therefore a level of indentation (A "Script" class that you just instantiate and instantly call is no better than a function in python) - but also splits into a sweeps_to_stills function that works on a loaded ExperimentList,reflection_table and one function run that does the parsing and loading.

Doing it here means that when you squash-merge it'll maintain all the attribution to your commit.

@ndevenish
Copy link
Member

I've also added the feature note for the CHANGELOG

@phyy-nx
Copy link
Member Author

phyy-nx commented Sep 12, 2019

Thanks @ndevenish

@phyy-nx phyy-nx merged commit 177a92f into master Sep 12, 2019
@phyy-nx phyy-nx deleted the ratchet branch September 12, 2019 14:54
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.

None yet

6 participants