search

cctbx / dxtbx

Diffraction Experiment Toolbox
BSD 3-Clause "New" or "Revised" License
2 stars 18 forks source link

Drop detectorbase from every format object #86

Open graeme-winter opened 4 years ago

graeme-winter commented 4 years ago

And instead provide a factory which will take a dxtbx format object which is fully populated and provide a detectorbase (if such a thing fits)

Also, from what I am aware detectorbase is used nowhere in dials or dxtbx so having this there only adds cost. Also as pointed https://github.com/cctbx/dxtbx/pull/84#issuecomment-530426117 many of these are imported and not used in e.g. dials.find_spots

nksauter commented 4 years ago

The initial understanding was that our collaborative effort on dxtbx would support LABELIT, regardless of whether detectorbase is used for DIALS. If the proposed code will continue to support all labelit and labelit_regression tests, then certainly this is OK.

Nick

Nicholas K. Sauter, Ph. D. Senior Scientist, Molecular Biophysics & Integrated Bioimaging Division Lawrence Berkeley National Laboratory 1 Cyclotron Rd., Bldg. 33R0345 Berkeley, CA 94720 (510) 486-5713

On Thu, Sep 12, 2019 at 6:54 AM Graeme Winter notifications@github.com wrote:

And instead provide a factory which will take a dxtbx format object which is fully populated and provide a detectorbase (if such a thing fits)

  • Would aid with things like non-square Bruker detectors (iotbx detectorbase insists they are square ISTR)
  • Would make debugging new detectors easier
  • Would mean that things which depend on detectorbase can do so with no new code, since (I assume) what they need is a subset of what we have in dxtbx

Also, from what I am aware detectorbase is used nowhere in dials or dxtbx so having this there only adds cost. Also as pointed #84 (comment) https://github.com/cctbx/dxtbx/pull/84#issuecomment-530426117 many of these are imported and not used in e.g. dials.find_spots

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/cctbx/dxtbx/issues/86?email_source=notifications&email_token=ADQ24VV3MLS4DATPC6I2VQTQJJCZDA5CNFSM4IWEQ3BKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HK73SXQ, or mute the thread https://github.com/notifications/unsubscribe-auth/ADQ24VWJRMWJARN5VKQ2PZ3QJJCZDANCNFSM4IWEQ3BA .

graeme-winter commented 4 years ago

@nksauter this is exactly the intent

Provide a factory function which will construct a detectorbase on the fly from a general format object, then remove the static one from every live instance

This may involve a small one off change in the labelit code base but nothing more than that

I’ll sketch out a proposal for discussion before embarking on any implementation

dagewa commented 4 years ago

:+1: for improving Bruker support. The related issue is #21. I'm not sure if it is detectorbase in that case, but It is indeed detectorbase in that case, limitations are applied in iotbx.detectors.BrukerImage