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

Process dataframe docstrings #27

Merged
merged 2 commits into from Dec 30, 2020
Merged

Conversation

ericmjl
Copy link
Collaborator

@ericmjl ericmjl commented Dec 30, 2020

@a-r-j requesting a short review on this one.

I added docstrings to the process_dataframe function, and slightly modified its implementation for simplicity.

One thing I'm a bit unsure of: if granularity == something shows up in two places. Do you think this can be simplified? "granularity" as a category of properties to worry about -- if it can be handled in one block of code rather than two, that would help with reasoning about the function and debugging/maintenance later on.

@ericmjl ericmjl requested a review from a-r-j December 30, 2020 16:00
@a-r-j a-r-j merged commit 49a6868 into graphein-api Dec 30, 2020
@a-r-j
Copy link
Owner

a-r-j commented Dec 30, 2020

LGTM, thanks Eric!

I think you're right about granularity - it's doing too much. The idea is to have control over:

  1. What the nodes are in a residue graph (e.g. a-Carbon "CA", b-Carbon "CB")
  2. Whether to use that atom position or compute residue centroids
  3. Whether or not to build a residue-graph as above or an atom-graph

@ericmjl
Copy link
Collaborator Author

ericmjl commented Dec 30, 2020

@a-r-j ok! That's definitely for another PR. Gonna raise an issue so that we can track this. 😄

@ericmjl ericmjl deleted the process_dataframe-docstrings branch December 30, 2020 20:03
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