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

Index refactor #1

Open
wants to merge 103 commits into
base: master
Choose a base branch
from
Open

Index refactor #1

wants to merge 103 commits into from

Conversation

rjgildea
Copy link
Owner

@rjgildea rjgildea commented May 31, 2019

  1. Refactor basis vector search code into strategies in dials.algorithms.indexing.basis_vector_search.strategies
    • fft1d, fft3d, real_space_grid_search
    • take as input a list of reciprocal lattice vectors and return a list of candidate basis vectors
  2. Refactor basis vector combination code into algorithms.indexing.basis_vector_search.combinations
    • Implemented as a sequence of iterators that can be chained together
    • candidate_orientation_matrices: finds suitable combinations of basis vectors in P1
    • filter_known_symmetry: filters output of previous to only accept those models that match known symmetry
    • filter_similar_orientations
  3. Refactor assign_indices to a strategy pattern in dials.algorithms.indexing.assign_indices
    • assign_indices_global, assign_indices_local
  4. Move non-primitive basis code to separate module dials.algorithms.indexing.non_primitive_basis with tests
  5. New dials.algorithms.indexing.model_evaluation
    • Move/rename SolutionTrackers: ModelRankWeighted, ModelRankFilter
    • move run_one_refinement inner function to new ModelEvaluation class
  6. Attempt to encapsulate handling of known symmetry into external class dials.algorithms.indexing.symmetry.SymmetryHandler, with tests
  7. New dials.algorithms.indexing.lattice_search module
    • Single BasisVectorSearch inheriting from indexer_base to consolidate previous fft1d, fft3d and real_space_grid_search classes. Code that is specific to a basis vector search now lives in this class instead of indexer_base.
    • Add simple test for BasisVectorSearch class
  8. Move basis vector optimisation code to new module dials.algorithms.indexing.basis_vector_search.optimise with tests
  9. Replace scan_range parameter with image_range
  10. Refactor command line script to a functional interface
    • add joint_indexing=False option to allow indexing multiple independent experiments in a single dials.index process
  11. Some refactoring of indexer.index() method to minimise code duplication with stills_indexer

jbeilstenedmands and others added 4 commits May 31, 2019 15:40
Please be advised that you can now install pre-commits by running

    libtbx.precommit install

This will fix up the code for you and avoids clutter getting into
the repository in the first place.
This is not currently well handled, and should instead be
handled with dials.reindex or symmetry/cosym
@ndevenish
Copy link

Checking with quality tools; there are some outright undefined variables; I checked the stills_indexer ones and these were moved out in this branch. The others might have existed anyway. This is obviously not covered by tests.

algorithms/indexing/model_evaluation.py:53:42: F821 undefined name 'difference_rotation_matrix_axis_angle'
algorithms/indexing/stills_indexer.py:118:20: F821 undefined name 'stills_indexer_fft3d'
algorithms/indexing/stills_indexer.py:120:20: F821 undefined name 'stills_indexer_fft1d'
algorithms/indexing/stills_indexer.py:122:20: F821 undefined name 'stills_indexer_real_space_grid_search'

Otherwise, there is a whole load of flake8 unused variables; I know not all these files have been completely rewritten but it'd be nice to get rid of the flake8 warnings; most of these are probably harmless, but I'm worried about cases where the variable should be used but isn't - is impossible for me to tell the difference...

algorithms/indexing/assign_indices.py:40:9: F841 local variable 'diffs' is assigned to but never used
algorithms/indexing/assign_indices.py:41:9: F841 local variable 'norms' is assigned to but never used
algorithms/indexing/assign_indices.py:42:9: F841 local variable 'hkl_ints' is assigned to but never used
algorithms/indexing/assign_indices.py:112:9: F841 local variable 'diffs' is assigned to but never used
algorithms/indexing/assign_indices.py:113:9: F841 local variable 'norms' is assigned to but never used
algorithms/indexing/assign_indices.py:114:9: F841 local variable 'hkl_ints' is assigned to but never used
algorithms/indexing/assign_indices.py:141:13: F841 local variable 'ref_sel' is assigned to but never used
algorithms/indexing/basis_vector_search/combinations.py:74:9: F841 local variable 'best_model' is assigned to but never used
algorithms/indexing/basis_vector_search/conftest.py:22:5: F841 local variable 'sg' is assigned to but never used
algorithms/indexing/basis_vector_search/strategies.py:267:9: F841 local variable 'sites_frac' is assigned to but never used
algorithms/indexing/basis_vector_search/strategies.py:282:9: F841 local variable 'orth' is assigned to but never used
algorithms/indexing/basis_vector_search/strategies.py:390:9: F841 local variable 'rlgrid' is assigned to but never used
algorithms/indexing/basis_vector_search/test_combinations.py:69:5: F841 local variable 'crystal_symmetry' is assigned to but never used
algorithms/indexing/indexer.py:831:17: F841 local variable 'isel' is assigned to but never used
algorithms/indexing/indexer.py:832:17: F841 local variable 'ref_panel' is assigned to but never used
algorithms/indexing/indexer.py:922:9: F841 local variable 'verbosity' is assigned to but never used
algorithms/indexing/model_evaluation.py:343:9: F841 local variable 'e' is assigned to but never used
algorithms/indexing/nearest_neighbor.py:96:9: F841 local variable 'centers' is assigned to but never used
algorithms/indexing/nearest_neighbor.py:138:9: F841 local variable 'fig' is assigned to but never used
algorithms/indexing/non_primitive_basis.py:30:9: F841 local variable 'cryst_orig' is assigned to but never used
algorithms/indexing/refinement.py:21:5: F841 local variable 'detector' is assigned to but never used
algorithms/indexing/stills_indexer.py:55:9: F841 local variable 'PX' is assigned to but never used
algorithms/indexing/stills_indexer.py:56:9: F841 local variable 'fig' is assigned to but never used
algorithms/indexing/stills_indexer.py:92:5: F841 local variable 'history' is assigned to but never used
algorithms/indexing/stills_indexer.py:590:21: F841 local variable 'crystal_model_nv0' is assigned to but never used
algorithms/indexing/stills_indexer.py:793:9: F841 local variable 'crystal_model_nv0' is assigned to but never used
algorithms/indexing/symmetry.py:161:5: F841 local variable 'supergroup' is assigned to but never used
algorithms/indexing/symmetry.py:163:5: F841 local variable 'triclinic_miller' is assigned to but never used
algorithms/indexing/symmetry.py:622:9: F841 local variable 'primitive_subsym' is assigned to but never used
algorithms/indexing/test_assign_indices.py:80:5: F841 local variable 'miller_set' is assigned to but never used
algorithms/indexing/test_index.py:52:9: F841 local variable 'result' is assigned to but never used
algorithms/indexing/test_index.py:120:9: F841 local variable 'result' is assigned to but never used
algorithms/indexing/test_index.py:151:9: F841 local variable 'result' is assigned to but never used
algorithms/indexing/test_index.py:226:9: F841 local variable 'result' is assigned to but never used
algorithms/indexing/test_index.py:263:9: F841 local variable 'result' is assigned to but never used
algorithms/indexing/test_index.py:300:9: F841 local variable 'result' is assigned to but never used
algorithms/indexing/test_index.py:337:9: F841 local variable 'result' is assigned to but never used
algorithms/indexing/test_index.py:372:9: F841 local variable 'result' is assigned to but never used
algorithms/indexing/test_index.py:403:9: F841 local variable 'result' is assigned to but never used
algorithms/indexing/test_index.py:438:9: F841 local variable 'result' is assigned to but never used
algorithms/indexing/test_index.py:476:9: F841 local variable 'result' is assigned to but never used
algorithms/indexing/test_index.py:515:13: F841 local variable 'result' is assigned to but never used
algorithms/indexing/test_index.py:563:5: F841 local variable 'result' is assigned to but never used
algorithms/indexing/test_index.py:607:5: F841 local variable 'result' is assigned to but never used
algorithms/indexing/test_index.py:633:5: F841 local variable 'result' is assigned to but never used
algorithms/indexing/test_index.py:777:5: F841 local variable 'experiments_list' is assigned to but never used
algorithms/indexing/test_lattice_search.py:58:5: F841 local variable 'indexed_reflections' is assigned to but never used
algorithms/indexing/test_lattice_search.py:99:5: F841 local variable 'indexed_reflections' is assigned to but never used
algorithms/indexing/test_max_cell.py:25:5: F841 local variable 'expts' is assigned to but never used
algorithms/indexing/test_symmetry.py:74:5: F841 local variable 'cs_new' is assigned to but never used
algorithms/indexing/test_symmetry.py:120:5: F841 local variable 'subgroups' is assigned to but never used

Forces users to run the installer instead of running code in place.
Resolves SCI-8659

Remove precommitbx environment
@rjgildea
Copy link
Owner Author

rjgildea commented Jun 4, 2019

@ndevenish have addressed the flake8 issues highlighted above

Fixes flake8 warning in per-image-analysis pull request
@rjgildea rjgildea force-pushed the index-refactor branch 2 times, most recently from 11b1a6a to d996c98 Compare June 5, 2019 13:55
ndevenish and others added 13 commits June 5, 2019 16:10
This directly sets the Agg canvas instead of relying on global state.

This fixes #795 by still allowing GUI plots if required by the user.
- Flake8
- Float imports
- Sort imports
Modify command line scripts which call these to catch and throw
a Sorry.
disables checks for things managed by black
due to recent changes in RLV. Some campsite improvements.
due to recent changes in RLV
Tidy up some bits of export too.
and update OpenSSL
@rjgildea rjgildea force-pushed the index-refactor branch 2 times, most recently from a2e17f9 to a0550d9 Compare June 7, 2019 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants