-
Notifications
You must be signed in to change notification settings - Fork 18
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
FormatROD #645
FormatROD #645
Conversation
I don't have time to study your changes at the moment. I recommend that you test on https://xrda.pdbj.org/entry/93 as well, which has a more complicated geometry. At the moment this class does not read axes, so you cannot index all sweeps in one go but each sweep should be indexable individually. |
Thanks, I'll take a look 👍 |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #645 +/- ##
==========================================
- Coverage 39.39% 39.10% -0.29%
==========================================
Files 178 179 +1
Lines 15519 15757 +238
Branches 2996 3040 +44
==========================================
+ Hits 6113 6162 +49
- Misses 8827 9012 +185
- Partials 579 583 +4 |
I confirm that after applying the file renaming workaround (#11 (comment)), all 14 datasets in |
I note the comment about porting |
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.
Now that you tested the code on several datasets, I think this is ready to be merged.
Thanks!
Non-essential suggestion:
How about mentioning the file name issue as a comment, or submiting an issue to the DIALS repository, so that someone who processes ROD datasets can find the workaround.
problems with dials.import.
Thanks! I put the issue in dxtbx, because that's where the template handling is found. But I added a note to the module docstring too. I'll wait to see if @graeme-winter has any other files to test before merging. |
Should Hypix 6000 data work?
It does not appear to have recognised that _9.thing goes with _10.thing etc. - I thought that was in this PR? I saw commits along these lines |
OK, working around that; please hold the line |
OK, I have a 7-scan run, which only partly indexes:
which suggests that there is at least one missed goniometry datum; testing joint=0 (this is for info not a blocker) |
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.
Firstly thank you for the changes - this has been on my to-do for a very long time, and I appreciate the work which went in to getting this done.
As mentioned in the review comment it feels slow to use but I would be the first to recognise working-but-slow is vastly superior to broken.
I also note that indexing a 7 sweep run did not work correctly, where indexing the sweeps one-at-a-time worked fine, so there is some geometric infelicity in the metadata reading (or writing) which would benefit from some attention in a subsequent PR
I would like this to be merged as is, I appreciate the addition of this capability; with the frame renumbering workaround from @biochem-fan this works nicely and adds real capability to DIALS.
I am still waiitng for the processing of my 7-scan run to finish but I am mid-way through integrating it so I am confident that this is a good change set.
epoch=None, | ||
) | ||
|
||
def get_raw_data(self): |
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.
By way of a narrative; spot finding feels slow which I suspect is down to the fact that the get_raw_data()
method is itself slower than the equivalent e.g. from CBF
or HDF5
data.
I mark this as an opportunity for future improvement perhaps as a C++ implementation rather than a bug here.
Thank you very much for testing on another dataset.
Indeed, it currently ignores all axes except the last one so I don't expect joint indexing works. |
End result:
I think this is most satisfactory |
Weirdly though:
|
@dagewa please could you try
or equivalent on any data set With this change enabled I think it tries to read |
etc. now fails on e.g. CBF or HDF5 data |
This looks like it's trying to read the |
This should handle this issue |
It did thank you |
Ah yes, good point. Thanks! |
Partial support for Rigaku Oxford Diffraction format images. --------- Co-authored-by: Takanori Nakane <nakane.t@gmail.com> Co-authored-by: Nicholas Devenish <ndevenish@gmail.com>
This is for #11.
FormatROD.py
consists of @biochem-fan's gist, edited to pass precommit checks and to change to a fasterunderstand
method.This has been tested to work with a couple of images:
The conversation at #11 and
FIXME
s in the code suggest that support remains partial, but it is a good start and I think worth adding to dxtbx at this point.