-
Notifications
You must be signed in to change notification settings - Fork 20
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
contributing guide: adding local neighborhood #161
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a partial review to give initial feedback and get started on the interactive process of suggestions and code changes. Some of these suggestions you can commit directly through the pull request interface and then pull locally using your git client.
I'll complete a more thorough review later.
.github/workflows/test-spras.yml
Outdated
path: docker-wrappers/LocalNeighborhood/. | ||
dockerfile: docker-wrappers/LocalNeighborhood/Dockerfile | ||
repository: sumedhars/local-neighborhood | ||
tags: v2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tags: v2 | |
tags: latest |
If you haven't explicitly tagged your image with a version, the default tag is latest
COPY ln-network.txt . | ||
COPY ln-nodes.txt . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to copy the data into the image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't necessary to commit these copies of the files that are in the docker-wrappers
directory, but it won't hurt anything either. We can leave it.
spras/local_neighborhood.py
Outdated
|
||
|
||
class LocalNeighborhood(PRM): | ||
required_inputs = ['network', 'nodetypes'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PathLinker has a "nodetypes" file because it had sources and targets. Local neighborhood only has one "type" of node, so we could consider calling this input file something different.
|
||
node_df = None | ||
|
||
if data.contains_node_columns('prize'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will need to review this logic more carefully. I didn't yet.
spras/local_neighborhood.py
Outdated
@param nodetypes: input node types with sources and targets (required) | ||
@param network: input network file (required) | ||
@param output_file: path to the output pathway file (required) | ||
@param k: path length (optional) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer have a parameter k
spras/local_neighborhood.py
Outdated
@param container_framework: choose the container runtime framework, currently supports "docker" or "singularity" (optional) | ||
""" | ||
# Add additional parameter validation | ||
# Do not require k |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Do not require k |
spras/local_neighborhood.py
Outdated
bind_path, network_file = prepare_volume(network, work_dir) | ||
volumes.append(bind_path) | ||
|
||
# PathLinker does not provide an argument to set the output directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove PathLinker comments
spras/local_neighborhood.py
Outdated
# mapped_out_prefix = mapped_out_dir + '/out' # Use posix path inside the container | ||
|
||
# print(mapped_out_prefix) | ||
#TODO: change for local neighborhood |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clean up TODOs
work_dir) | ||
print(out) | ||
|
||
# Rename the primary output file to match the desired output filename |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove if not needed
Closing this because it served its purpose for the summer 2024 introduction to SPRAS. |
No description provided.