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

Deprecate datablock.*Diff pseudoclasses #302

Merged
merged 5 commits into from
Mar 3, 2021
Merged

Deprecate datablock.*Diff pseudoclasses #302

merged 5 commits into from
Mar 3, 2021

Conversation

Anthchirp
Copy link
Member

@Anthchirp Anthchirp commented Feb 12, 2021

BeamDiff, ScanDiff, GoniometerDiff, DetectorDiff, SequenceDiff in dxtbx.datablock are replaced by functions in dxtbx.model.compare (beam_diff, etc.)

They don't really have anything to do with datablocks or experiment lists.

@Anthchirp
Copy link
Member Author

Question here: There are a bunch more of these comparator "classes" (BeamDiff, ScanDiff, GoniometerDiff, DetectorDiff) in dxtbx.datablock. Should these go into dxtbx.model.experiment_list, or elsewhere collectively, eg. dxtbx.model.compare?

They don't really have anything to do with datablocks or experiment lists.

@codecov
Copy link

codecov bot commented Feb 12, 2021

Codecov Report

Merging #302 (ebf6549) into main (8cdb7d3) will increase coverage by 0.00%.
The diff coverage is 10.00%.

@@           Coverage Diff           @@
##             main     #302   +/-   ##
=======================================
  Coverage   46.73%   46.74%           
=======================================
  Files         232      233    +1     
  Lines       19207    19221   +14     
  Branches     2763     2764    +1     
=======================================
+ Hits         8977     8985    +8     
- Misses       9733     9739    +6     
  Partials      497      497           

@phyy-nx
Copy link

phyy-nx commented Feb 25, 2021

Looks like no impact on my side if these classes are moved.

Base automatically changed from master to main March 1, 2021 10:53
@Anthchirp Anthchirp marked this pull request as ready for review March 3, 2021 10:38
@Anthchirp Anthchirp changed the title Deprecate SequenceDiff pseudoclass Deprecate datablock.*Diff pseudoclasses Mar 3, 2021
to fix the Azure build
@Anthchirp
Copy link
Member Author

@graeme-winter You self-requested a review on this, I remember it had something to do with tolerance phil thingies. Do you want to add your 2p here or is this a separate PR?

Otherwise this here is done - tests pass and everything.

@graeme-winter
Copy link
Collaborator

@graeme-winter You self-requested a review on this, I remember it had something to do with tolerance phil thingies. Do you want to add your 2p here or is this a separate PR?

Otherwise this here is done - tests pass and everything.

Warning: Ambiguous parameter definition: wavelength = 0.979499
Best matches:
  input.tolerance.beam.wavelength
  geometry.beam.wavelength
Assuming geometry.beam.wavelength was intended.

I think that this did not used to be the case? 🤔

@graeme-winter
Copy link
Collaborator

@graeme-winter You self-requested a review on this, I remember it had something to do with tolerance phil thingies. Do you want to add your 2p here or is this a separate PR?
Otherwise this here is done - tests pass and everything.

Warning: Ambiguous parameter definition: wavelength = 0.979499
Best matches:
  input.tolerance.beam.wavelength
  geometry.beam.wavelength
Assuming geometry.beam.wavelength was intended.

I think that this did not used to be the case? 🤔

Grey-Area override :) $ dials.import ../insu_7_testbeamline_2.nxs origin=-82.7366,77.3027,-332.998
Sorry: Ambiguous parameter definition: origin = -82.7366,77.3027,-332.998
Best matches:
  input.tolerance.detector.origin
  geometry.detector.panel.origin
  geometry.detector.hierarchy.origin
  geometry.detector.hierarchy.group.origin

There we go... I think this was what I was meaning - I think this is somewhat related but I am minded to make that a different pull request.

Copy link
Collaborator

@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.

Scanned change set, did not see anything which would make future debuging harder and moving away from pseudoclasses is always a good thing, thank you for the work here.

Am slightly concerned by use of functools.partial as an indicator of poor overall design, but that is out of context for this change set. I will raise an issue with my other concerns as raised in the conversation.

@Anthchirp Anthchirp merged commit 3d13188 into main Mar 3, 2021
@Anthchirp Anthchirp deleted the deonion branch March 3, 2021 17:03
Anthchirp added a commit that referenced this pull request Apr 22, 2021
Previously deprecated in #302
Anthchirp added a commit that referenced this pull request Apr 23, 2021
Remove deprecated classes (previously deprecated in #302)
  dxtbx.datablock.*Diff
  dxtbx.model.experiment_list.SequenceDiff

Remove deprecated function (previously deprecated in #316)
  dxtbx.serialize.load.imageset_from_string
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.

3 participants