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

Deprecate ContactMap (allowing rename ContactFrequency => ContactMap) #84

Merged
merged 2 commits into from
Aug 14, 2020

Conversation

dwhswenson
Copy link
Owner

First step of #82.

Also, note that the correct warning to use here is FutureWarning, which will actually show for users (DeprecationWarning will not). I learned that from this SO answer, written by a very brilliant author. I would argue that basic interactive use of Contact Map Explorer in a Jupyter notebook counts as "application", not "library."

@dwhswenson dwhswenson changed the title Deprecate ContactMap (so we can rename ContactFrequency => ContactMap) Deprecate ContactMap (allowing rename ContactFrequency => ContactMap) Aug 14, 2020
@dwhswenson dwhswenson added deprecation addition of deprecation warnings misc PR labels Aug 14, 2020
@dwhswenson
Copy link
Owner Author

Codeclimate complains about:

  • contact_map.py being too long (well, it'll be shorter when we remove ContactMap! plus, as stated before, I intend to split things out of it in a PR dedicated to that process)
  • the changes for deprecating classmethods look like repeated code. While writing the code, I actually thought about writing a decorator to avoid that repetition, but it seemed like overkill for code that's going to be removed in a near-future release.

Marking both as wontfix.

Copy link
Collaborator

@sroet sroet left a comment

Choose a reason for hiding this comment

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

Looks good in general,

small comments on the examples:

  • in the contact_map.ipynb there are still a couple references to ContactMap (an import and some text in the list residue contacts section
  • The without_atom_slice notebook still references contactMap (as the only bad case), so there might need to be some alteration of the text there (as the example can't get our code to be slower than without atomslice). Maybe try adding a map for a single frame?
    frame_contacts = ContactFrequency(traj[0], query=used_atoms, haystack=used_atoms)

contact_map/contact_map.py Show resolved Hide resolved
@dwhswenson
Copy link
Owner Author

  • in the contact_map.ipynb there are still a couple references to ContactMap (an import and some text in the list residue contacts section

Fixed.

  • The without_atom_slice notebook still references contactMap (as the only bad case), so there might need to be some alteration of the text there (as the example can't get our code to be slower than without atomslice). Maybe try adding a map for a single frame?

I probably should have actually read the text instead of just removing every bit of code that said ContactMap.... anyway, I think what's there now illustrates the original point.

Copy link
Collaborator

@sroet sroet left a comment

Choose a reason for hiding this comment

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

LGTM, feel free to merge

@dwhswenson dwhswenson merged commit 1fb5d85 into master Aug 14, 2020
@dwhswenson dwhswenson deleted the deprecate_map branch August 14, 2020 12:40
@dwhswenson dwhswenson mentioned this pull request Sep 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation addition of deprecation warnings misc PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants