Skip to content

replace parsing of $CLIBD/symop.lib with sgtbx#41

Merged
rjgildea merged 3 commits intomasterfrom
sgtbx
May 14, 2019
Merged

replace parsing of $CLIBD/symop.lib with sgtbx#41
rjgildea merged 3 commits intomasterfrom
sgtbx

Conversation

@rjgildea
Copy link
Contributor

No description provided.

@rjgildea rjgildea requested a review from graeme-winter May 13, 2019 16:34
@graeme-winter
Copy link
Contributor

Looked through the code changes, they all look sensible. Ran a test, seems to do what I would expect with the code in the branch. One concern I have is the handling of R3:H vs. H3 etc. - I know this can be a thing. Have you a test case for this?

@graeme-winter
Copy link
Contributor

I do note that this now means fast_dp does not run on Python3 - possibly it could do? Maybe an absolute imports thing.

Copy link
Contributor

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

Definite improvement, works for the use cases which were causing issues last week. Also tested with fast_rdp which seemed to do "the right thing"

@graeme-winter
Copy link
Contributor

Travis test failure reflects previous lack of testing not a new problem - the issue is highlighted by the adding of tests.

@Anthchirp
Copy link
Contributor

fast_dp will not run on python3 as it now depends on cctbx more than it did before.

@rjgildea
Copy link
Contributor Author

I do note that this now means fast_dp does not run on Python3 - possibly it could do? Maybe an absolute imports thing.

fast_dp will not run on python3 as it now depends on cctbx more than it did before.

As far as I can tell fast_dp already depended on sgtbx in a number of places, e.g. when parsing the pointless xml output:

pointgroup = (
sgtbx.space_group_type(lauegroup)
.group()
.build_derived_acentric_group()
.type()
.number()
)

or when the --cell option is set:

def generate_primitive_cell(unit_cell_constants, space_group_name):
"""For a given set of unit cell constants and space group, determine the
corresponding primitive unit cell..."""
uc = unit_cell(unit_cell_constants)
sg = space_group(space_group_symbols(space_group_name).hall())
cs = symmetry(unit_cell=uc, space_group=sg)
csp = cs.change_basis(cs.change_of_basis_op_to_primitive_setting())
return csp.unit_cell()

@rjgildea
Copy link
Contributor Author

@graeme-winter r.e.

One concern I have is the handling of R3:H vs. H3 etc. - I know this can be a thing. Have you a test case for this?

$ dials.python
Python 2.7.16 (default, Apr  3 2019, 15:01:33) 
[GCC 4.4.7 20120313 (Red Hat 4.4.7-23)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from fast_dp import cell_spacegroup
>>> cell_spacegroup.check_spacegroup_name("H3")
'R 3 :H'
>>> cell_spacegroup.check_spacegroup_name("R3:H")
'R 3 :H'
>>> cell_spacegroup.ersatz_pointgroup("H3")
'R 3'

Is this the behaviour you expect?

@graeme-winter
Copy link
Contributor

@rjgildea not sure, will review what it used to do and get back to you

@graeme-winter
Copy link
Contributor

Reviewing

>>> cs.ersatz_pointgroup('H3')
'R3'
>>> cs.ersatz_pointgroup('R32')
'R32'
>>> cs.ersatz_pointgroup('H32')
'R321'
>>> cs.ersatz_pointgroup('H3')
'R3'
>>> reload(cs)
<module 'fast_dp.cell_spacegroup' from '/Users/graeme/svn/cctbx/modules/fast_dp/fast_dp/cell_spacegroup.py'>
>>> cs.ersatz_pointgroup('H3')
'R 3'
>>> cs.ersatz_pointgroup('R32')
'R 3 2'
>>> cs.ersatz_pointgroup('H32')
'R 3 2'

reload was checking out sgtbx branch - first 3 calls on master. Looks like the end game is more spaces and fewer rustic bugs, so happy.

@rjgildea rjgildea merged commit d13fcb0 into master May 14, 2019
@rjgildea rjgildea deleted the sgtbx branch May 14, 2019 09:40
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