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

Format.get_mask() -> get_static_mask() #73

Merged
merged 17 commits into from
Aug 29, 2019
Merged

Format.get_mask() -> get_static_mask() #73

merged 17 commits into from
Aug 29, 2019

Conversation

rjgildea
Copy link

This pull requests resolves #70 (see also #65).

  • Add an optional Format.get_static_mask() method to allow format classes to define a static mask to be used on all images.
  • Add tests for static masks in FormatHDF5SaclaMPCCD and FormatPYunspecifiedStill.
  • Remove superfluous Format.get_mask() functions - these are no longer called and the functionality they used to provide are provided elsewhere.

@rjgildea
Copy link
Author

One remaining issue is that as far as I can tell, FormatNexus and FormatHDF5EigerNearlyNexus define a get_mask() method that looks up a per-image mask defined in the Nexus file. I'm not sure how this would be handled in the current framework. Do we have any such example Nexus files? @graeme-winter?

dxtbx/format/FormatNexus.py

Lines 130 to 131 in 9d89b3a

def get_mask(self, index=None, goniometer=None):
return MaskFactory(self.instrument.detectors, index).mask

dxtbx/format/FormatHDF5EigerNearlyNexus.py

Lines 402 to 403 in 9d89b3a

def get_mask(self, index=None, goniometer=None):
return MaskFactory(self.instrument.detectors, index).mask

@rjgildea
Horrible hack to work around format_instance caching
c04fe3a 
This is needed to prevent an incorrect format_instance affecting
other tests that are run after this one.
@rjgildea
Don't override external_lookup.mask if already set
7dbd7bc 
If the format class has already specified a static mask then combine
with any externally-provided mask instead of simply replacing.
@rjgildea
Remove superfluous FormatClass.get_mask() calls
bf2a6cf
jmp1985
jmp1985 approved these changes Aug 27, 2019
View reviewed changes
Copy link

@jmp1985 jmp1985 left a comment

Choose a reason for hiding this comment

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

The changes look good to me.

Regarding the Nexus get_mask thing, as far as I am aware nothing actually uses this functionality.

@rjgildea rjgildea marked this pull request as ready for review August 27, 2019 09:36
@biochem-fan
Copy link
Member

Great. And thank you very much for adding a test for the MPCCD class.

Could you please add a test to make sure you can combine the dxtbx mask with a user provided mask?

@rjgildea
Copy link
Author

Could you please add a test to make sure you can combine the dxtbx mask with a user provided mask?

Done (and found a bug): 6130e74#diff-195e009be36221a969d4a55a0f00e072R274

biochem-fan
biochem-fan approved these changes Aug 27, 2019
View reviewed changes
Copy link
Member

@biochem-fan biochem-fan left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test for combining the masks.

phyy-nx
phyy-nx approved these changes Aug 29, 2019
View reviewed changes
Copy link

@phyy-nx phyy-nx left a comment

Choose a reason for hiding this comment

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

Looks good. Happy with this version.

@rjgildea rjgildea merged commit 967a7bf into master Aug 29, 2019
@rjgildea rjgildea mentioned this pull request Aug 29, 2019
Closed
@Anthchirp Anthchirp deleted the dxtbx-70 branch August 29, 2019 20:37
Anthchirp added a commit that referenced this pull request Apr 6, 2020
@Anthchirp
Remove xfailing test added in #73
fe0494b 
Test has never passed, and test file will not load on Python 3 anyway
@graeme-winter graeme-winter mentioned this pull request Jul 15, 2020
Closed
graeme-winter added a commit that referenced this pull request Jul 15, 2020
@graeme-winter
Fix mask pointers
e6ca5d8 
Following #73 get_mask should have been
renamed get_static_mask in order for it to be called. This change set will
fix dials/dials#1340
@graeme-winter graeme-winter mentioned this pull request Jul 15, 2020
Merged
graeme-winter added a commit that referenced this pull request Aug 5, 2020
@graeme-winter
Fix mask pointers: get_mask -> get_static_mask (#198)
65c1cdc 
Following #73 get_mask should have been
renamed get_static_mask in order for it to be called. This change set will
fix dials/dials#1340

* Fix mask pointers
* Fix mask array dimensions - refer to the actual data set if 1 module, else stick with existing behaviour
- frequently the data_size information in the HDF5 meta is inaccurate.
* Add tool to show mask information, with test
* Iterate through imagesets not experiments - the latter can be very numerous for large grid scans and contains a collection of the same static mask...

Thanks to @rjgildea for PR feedback
ndevenish pushed a commit that referenced this pull request Aug 5, 2020
@graeme-winter @ndevenish
Fix mask pointers: get_mask -> get_static_mask (#198)
ecfd391 
Following #73 get_mask should have
been renamed get_static_mask in order for it to be called. This change
set will fix dials/dials#1340

* Fix mask pointers
* Fix mask array dimensions - refer to the actual data set if 1 module,
  else stick with existing behaviour - frequently the data_size
  information in the HDF5 meta is inaccurate.
* Add tool to show mask information, with test
* Iterate through imagesets not experiments - the latter can be very
  numerous for large grid scans and contains a collection of the same
  static mask...

Thanks to @rjgildea for PR feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@biochem-fan biochem-fan biochem-fan approved these changes

@jmp1985 jmp1985 jmp1985 approved these changes

@phyy-nx phyy-nx phyy-nx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Format needs get_static_mask
5 participants
@rjgildea @biochem-fan @jmp1985 @phyy-nx @Anthchirp