Skip to content

Fix for lack of unitcell; better colorbar scaling#64

Merged
sroet merged 1 commit intodwhswenson:masterfrom
nffaruk:master
Nov 17, 2019
Merged

Fix for lack of unitcell; better colorbar scaling#64
sroet merged 1 commit intodwhswenson:masterfrom
nffaruk:master

Conversation

@nffaruk
Copy link
Contributor

@nffaruk nffaruk commented Nov 11, 2019

The colorbar parameters are taken from here to provide better scaling to square aspect ratio plots (which I also think should be the default aspect ratio)

Copy link
Owner

@dwhswenson dwhswenson left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

Looks good to me -- I'll leave it up for another 24 hours or so, just in case @sroet has any comment or question. I did a quick local checkout and ran the main example, and it still looked fine.

One question: was there anything specific about your use case that led to this problem? (other than using a square aspect ratio?) I'm asking so that, in case we ever come back to revisit this approach, we can be sure not to break what you've fixed!

As to whether we should switch to a square aspect ratio by default -- that's a reasonable suggestion. We should definitely think about it. Using the matplotlib default aspect ratio is mostly based on thinking about best filling the space in the journal column (and laziness). A square default makes a lot of sense, though (if haystack and query aren't identical, the box for each contact pair still won't be square, but the most common use case is haystack == query).

@sroet
Copy link
Collaborator

sroet commented Nov 12, 2019

Looks good to me as well.

If it is not to much to ask, I would really like to also see a test. Just to make sure that this behavior does not regress.

@nffaruk
Copy link
Contributor Author

nffaruk commented Nov 12, 2019

One question: was there anything specific about your use case that led to this problem? (other than using a square aspect ratio?) I'm asking so that, in case we ever come back to revisit this approach, we can be sure not to break what you've fixed!

Thanks for the approval - in my case my research group has it's own CG MD software that does not have solvent and thus no unit cells, and so return md.Trajectory(...) in def _atom_slice(...) was throwing UnboundLocalError: local variable 'unitcell_lengths' referenced before assignment - the unit cell kwargs are set to None by default in the mdtraj API for the Trajectory class by the way.

A more general case where others might run into this includes PDB files with dummy CRYST1 records (which mdtraj recognizes and removes) or ones that don't have any such records (eg. curated benchmark sets for folding and docking).

If it is not to much to ask, I would really like to also see a test. Just to make sure that this behavior does not regress.

I think it would be as simple as making a copy of the .../contact_map/contact_map/tests/trajectory.pdb without the CRYST1 lines. The calculated contacts may not be the same as before because mdtraj.compute_neighbors(...) will not treat it as periodic.

@sroet
Copy link
Collaborator

sroet commented Nov 17, 2019

Written a test that confirms this fixes the described error #65. Merging this in.

Thank you very much for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants