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.spot_counts_per_image: explicitly fail given indexed reflections #1690

Merged
merged 5 commits into from
May 24, 2021

Conversation

rjgildea
Copy link
Contributor

@rjgildea rjgildea commented May 7, 2021

Since behaviour isn't currently well-defined on indexed reflections, ensure that we are only given output of spotfinding.

See also #1689

rjgildea and others added 3 commits May 7, 2021 09:44
Since behaviour isn't currently well-defined on indexed reflections,
ensure that we are only given output of spotfinding.
@rjgildea rjgildea closed this May 7, 2021
@graeme-winter graeme-winter reopened this May 7, 2021
@graeme-winter
Copy link
Contributor

The proper fix for this involves resolving #1029 which in turn will generate a lot of work around every step downstream of the indexing. In the interim, this pull request would resolve the issue of user surprise which would be an improvement.

If we can resolve the underlying question then the changes in here may need to be reverted, but experience suggests that this is unlikely to happen in short order so justifies @rjgildea work here.

Copy link
Contributor

@graeme-winter graeme-winter left a comment

Choose a reason for hiding this comment

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

I am not happy that this is the best solution, but I respect the point that this is the most pragmatic resolution to the situation we find ourselves in, thanks for making the changes.

I'll tag this commit once merged on #1029 as something which may need to be reverted if we get that far.

@@ -67,6 +67,12 @@ def run(args=None):
sys.exit("Only one reflection list may be passed")
reflections = reflections[0]

if "miller_index" in reflections:
Copy link
Contributor

Choose a reason for hiding this comment

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

if "miller_index" in reflctions or any(experiments.crystals())

-> catch both outcomes (because I can see someone passing indexed.refl imported.expt or something in response to one of the cases)

if both then maybe

sys.exit("Only unindexed experiments / reflections are currently supported")

So you catch this in one go?

@ndevenish ndevenish merged commit 5c023e5 into main May 24, 2021
@ndevenish ndevenish deleted the spot_counts_fail_indexed branch May 24, 2021 14:55
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

4 participants