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

Update __init__.py #528

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

Update __init__.py #528

wants to merge 3 commits into from

Conversation

tomburnley
Copy link
Contributor

Update ensemble_refinement to 2020 version. Added DEN restraints (now used by default). Fixed refinement flag manager. Added support for importing external TLS parameters from additional PDB.

Update ensemble_refinement to 2020 version.  Added DEN restraints (now used by default).  Fixed refinement flag manager.  Added support for importing external TLS parameters from additional PDB.
@nwmoriarty
Copy link
Contributor

nwmoriarty commented Sep 16, 2020 via email

@tomburnley
Copy link
Contributor Author

tomburnley commented Sep 16, 2020 via email

Copy link
Contributor

@olegsobolev olegsobolev left a comment

Choose a reason for hiding this comment

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

Just a note, there are bunch of functions of the model class that deal with TLS:
get_header_tls_selections()
extract_tls_selections_from_input()
determine_tls_groups(self, selection_strings, generate_tlsos)

I see a lot of code to deal with TLS in this particular file, so maybe something more is going on, so I would not insist on using them in this particular case instead of things like

self.model.tls_groups.tlsos = fit_tlsos
fit_tlsos = [tls_tools.tlso(t=o.t, l=o.l, s=o.s, origin=o.origin) for o in tls_params.tls_params]

mmtbx/refinement/ensemble_refinement/__init__.py Outdated Show resolved Hide resolved
mmtbx/refinement/ensemble_refinement/__init__.py Outdated Show resolved Hide resolved
make_header("Import External TLS", out = self.log)
print('External TLS model: ' + self.params.import_tls_pdb, file=self.log)
pdb_import_tls = self.params.import_tls_pdb
pdb_tls_inp = iotbx.pdb.hierarchy.input(file_name=pdb_import_tls)
Copy link
Contributor

Choose a reason for hiding this comment

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

please use iotbx.pdb.input() instead, preferably checking if the file exists.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still this, with the explanation in the previous message.

mmtbx/refinement/ensemble_refinement/__init__.py Outdated Show resolved Hide resolved
Move all import statements to header.  Remove duplicate self.assign_solvent_tls_groups() in Import TLS from reference model.
@bkpoon
Copy link
Member

bkpoon commented Sep 20, 2020

There are unused imports causing the Checks tests to fail.

In file __init__.py:
time imported at line 34
miller imported at line 19
geometry_restraints imported at line 17
cdl_utils imported at line 636
xray imported at line 21

An update will cause the CI to run again and the error for Full and Base should go away since I checked in some updates to the master branch.

Remove unused imports
@bkpoon
Copy link
Member

bkpoon commented Sep 22, 2020

Thanks! The Ci branch failures are fine. That pipeline does not run on the final, merged code with the updates to master.

@olegsobolev
Copy link
Contributor

This requested change seems to be missed:

please use iotbx.pdb.input() instead of iotbx.pdb.hierarchy.input(), preferably checking if the file exists.

@tomburnley
Copy link
Contributor Author

This requested change seems to be missed:

please use iotbx.pdb.input() instead of iotbx.pdb.hierarchy.input(), preferably checking if the file exists.

Can you explain the need for this and point to an example of use I can copy from please.

@olegsobolev
Copy link
Contributor

@tomburnley , iotbx.pdb.hierarchy.input() seems to be a very thin wrapper that pretty much constructs hierarchy and keep input object:

class input(input_hierarchy_pair):
"""
Class used for reading a PDB hierarchy from a file or string.
Attributes
----------
input : iotbx.pdb.pdb_input_from_any
hierarchy : iotbx.pdb.hierarchy.root
Examples
--------
>>> import iotbx.pdb.hierarchy
>>> pdb_in = iotbx.pdb.hierarchy.input(pdb_string='''
... ATOM 1 N ASP A 37 10.710 14.456 9.568 1.00 15.78 N
... ATOM 2 CA ASP A 37 9.318 14.587 9.999 1.00 18.38 C
... ''')
>>> print pdb_in.hierarchy.atoms_size()
2
"")
"""
def __init__(self, file_name=None,
pdb_string=None, source_info=Auto, sort_atoms=True):
"""
Initializes an input from a file or string.
Parameters
----------
file_name : str, optional
pdb_string : str, optional
source_info : str, optional
Indicates where this PDB came from (i.e. "string")
"""
assert [file_name, pdb_string].count(None) == 1
import iotbx.pdb
if (file_name is not None):
assert source_info is Auto
pdb_inp = iotbx.pdb.input(file_name=file_name)
else:
if (source_info is Auto): source_info = "string"
pdb_inp = iotbx.pdb.input(
source_info=source_info, lines=flex.split_lines(pdb_string))
super(input, self).__init__(input=pdb_inp, sort_atoms=sort_atoms)
# END_MARKED_FOR_DELETION_OLEG

The original reason to create an extra layer for such a simple task is not clear to me in the first place. I could very well miss something, so clarifications from anyone are welcomed. As of now, I believe this whole layer better be removed, therefore advocate against using it anywhere.

In practical terms regarding this pull request I suggest to change this:

pdb_tls_inp = iotbx.pdb.hierarchy.input(file_name=pdb_import_tls)
tls_params = pdb_tls_inp.input.extract_tls_params(pdb_tls_inp.hierarchy)

to this:

pdb_tls_inp = iotbx.pdb.input(file_name=pdb_import_tls)
tls_params = pdb_tls_inp.extract_tls_params(pdb_tls_inp.construct_hierarchy())

Other examples of iotbx.pdb.input() usage can be found e.g. in iotbx/command_line/pdb_labels_comparison.py and many other files across the repository.

tomburnley added a commit that referenced this pull request Jun 21, 2021
Updated version of ensemble refinement to include den restraints and echt b-factor model to accompany paper submitted to Acta D.  Tested with phenix-1.19.2-4158.  Issues with previous pull request (#528 (comment)) fixed.
nwmoriarty pushed a commit that referenced this pull request Jun 22, 2021
Updated version of ensemble refinement to include den restraints and echt b-factor model to accompany paper submitted to Acta D.  Tested with phenix-1.19.2-4158.  Issues with previous pull request (#528 (comment)) fixed.
russell-taylor pushed a commit to ReliaSolve/cctbx_project that referenced this pull request Jul 13, 2021
Updated version of ensemble refinement to include den restraints and echt b-factor model to accompany paper submitted to Acta D.  Tested with phenix-1.19.2-4158.  Issues with previous pull request (cctbx#528 (comment)) fixed.
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.

5 participants