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

FIxing modeller bug for WT case #1074

Merged
merged 4 commits into from Aug 25, 2018
Merged

FIxing modeller bug for WT case #1074

merged 4 commits into from Aug 25, 2018

Conversation

steven-albanese
Copy link
Member

Modeller was throwing an error when given WT in the yaml pipeline.

  • fixing the way this is being handled
  • Adding a test for the WT case

@steven-albanese
Copy link
Member Author

Tests are passing, but I'm not sure if I've got the test for the WT case correct

Copy link
Contributor

@andrrizzi andrrizzi left a comment

Choose a reason for hiding this comment

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

The fix looks good! For the test, I wouldn't add a new one but just add to test_modeller_mutations something like

# We haven't set the strip_protons options, so this shouldn't do anything
exp_builder._db._setup_molecules(mol_id)
assert not os.path.exists(output_path)

# ADD THIS BLOCK
# Calling modeller with WT creates a file (although the protein is not mutated).
exp_builder._db.molecules[mol_id]['pdbfixer'] = {
    'apply_mutations' : {
        'chain_id' : 'A',
        'mutations': 'WT',
    }
}
setup_molecule_output_check(exp_builder._db, mol_id, output_path)
os.remove(output_path)  # Remove file for next check.
# NEW BLOCK ENDS

# Now we set the strip_protons options and repeat
exp_builder._db.molecules[mol_id]['modeller'] = {
    'apply_mutations': {
        'chain_id': 'A',
        'mutations': 'T85I',
    }
}
setup_molecule_output_check(exp_builder._db, mol_id, output_path)

@andrrizzi
Copy link
Contributor

Whops! Sorry, it looks like the test I suggested fails because when using WT with modeller it doesn't save the pdb file. If that's expected, feel free to substitute

setup_molecule_output_check(exp_builder._db, mol_id, output_path)
os.remove(output_path)  # Remove file for next check.

with

exp_builder._db._setup_molecules(mol_id)
assert not os.path.exists(output_path)

Can you also update the release history (docs/whatsnew.rst) before merging?

@steven-albanese
Copy link
Member Author

Hmm weirdly, it looks like it's actually the second setupt_molecule_output_check that is failing:

setup_molecule_output_check(exp_builder._db, mol_id, output_path)

Do I need to reinitialize the ExperimentBuilder object in between the wild type and mutant case?

@andrrizzi
Copy link
Contributor

Hmm. I don't think you should! Can you check what exp_builder._db.is_molecule_setup(mol_id) returns after the call to os.remove(output_path)?

@andrrizzi
Copy link
Contributor

Tests finally pass! Should I merge this and go ahead with a bugfix release?

@steven-albanese
Copy link
Member Author

steven-albanese commented Aug 24, 2018 via email

@andrrizzi andrrizzi merged commit 385f0a8 into master Aug 25, 2018
@andrrizzi andrrizzi deleted the modeller branch August 25, 2018 21:02
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.

None yet

2 participants