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

Evcomplex2 #225

Open
wants to merge 106 commits into
base: develop
Choose a base branch
from
Open

Evcomplex2 #225

wants to merge 106 commits into from

Conversation

aggreen
Copy link
Contributor

@aggreen aggreen commented Oct 8, 2019

This pull request includes all the functions used for calculations in Green & Elhabashy, et al. https://www.biorxiv.org/content/10.1101/791293v1

aggreen added 30 commits January 2, 2018 14:16
Copy link
Contributor

@thomashopf thomashopf left a comment

Choose a reason for hiding this comment

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

Almost complete review, still missing are:

  • evcouplings/couplings/protocol.py
  • evcouplings/couplings/pairs.py

All small-ish stuff except for evcouplings/compare/protocol.py - I think most of the additions there regarding the machine learning models do not belong into the comparison stage which should really just compare results to experiments. I think this stuff belongs more into the complex submodule, and maybe means another stage for the complex pipeline is needed?

config/sample_config_complex.txt Outdated Show resolved Hide resolved
uniprot_to_embl_table: /n/groups/marks/databases/complexes/idmapping/idmapping_uniprot_embl_2017_02.txt
ena_genome_location_table: /n/groups/marks/databases/complexes/ena/2017_02/cds_pro.txt
structurefree_model_file: /n/groups/marks/users/agreen/dev/EVcouplings/evcouplings/compare/aux/residue_strucfree.saved
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have these somewhere publicly accessible

config/sample_config_complex.txt Outdated Show resolved Hide resolved
evcouplings/compare/asa.py Outdated Show resolved Hide resolved
evcouplings/compare/asa.py Outdated Show resolved Hide resolved
temp_dataframe = id_dataframe.copy()

id_strings = [_split_annotation_string(x) for x in temp_dataframe.id_1]
temp_dataframe["id_string_1"], temp_dataframe["r_s_1"], temp_dataframe["r_e_1"] = \
Copy link
Contributor

Choose a reason for hiding this comment

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

not a big fan of new lines with backslash - nicer to wrap first part of assignment in (...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't picture what you mean here, sorry, was only using the slash because the assignment doesn't seem to nicely support wrapping with ()

evcouplings/complex/protocol.py Show resolved Hide resolved
evcouplings/complex/protocol.py Outdated Show resolved Hide resolved
evcouplings/couplings/mapping.py Outdated Show resolved Hide resolved
evcouplings/couplings/mapping.py Outdated Show resolved Hide resolved
@thomashopf
Copy link
Contributor

Plan for getting this into develop:

  1. Current develop branch will become master and a new release very soon (v0.0.6)
  2. Then merge this PR into develop, perform testing if stable, then will become v0.0.7

Copy link
Contributor

@thomashopf thomashopf left a comment

Choose a reason for hiding this comment

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

Here is review of second bit - major comments:

  1. Please refactor complex_mean_field() and mean_field() like done for plm-based protocols (See detailed comments above)

  2. the _postprocess_inference() method should stay as it is (may need making the pymol creation functions more generic to handle segments as well)

evcouplings/couplings/pairs.py Outdated Show resolved Hide resolved
evcouplings/couplings/pairs.py Outdated Show resolved Hide resolved
evcouplings/couplings/pairs.py Outdated Show resolved Hide resolved
evcouplings/couplings/pairs.py Outdated Show resolved Hide resolved
evcouplings/couplings/pairs.py Outdated Show resolved Hide resolved
evcouplings/couplings/protocol.py Show resolved Hide resolved
evcouplings/couplings/protocol.py Outdated Show resolved Hide resolved
evcouplings/couplings/protocol.py Outdated Show resolved Hide resolved
evcouplings/couplings/protocol.py Outdated Show resolved Hide resolved
evcouplings/couplings/protocol.py Outdated Show resolved Hide resolved
@aggreen
Copy link
Contributor Author

aggreen commented Oct 16, 2020

Reminder: check that Issue #252 is resolved

@aggreen
Copy link
Contributor Author

aggreen commented Dec 23, 2020

The new changes pushed up until this point address major and minor comments including the mean field refactoring. Also includes a fix for #252 (30a8963) - typo in commit says #225.

Still TODO:

  1. Refactoring of scoring code into another step of the pipeline as discussed previously

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.

4 participants