-
Notifications
You must be signed in to change notification settings - Fork 24
feat: netmix2 #225
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
base: main
Are you sure you want to change the base?
feat: netmix2 #225
Conversation
@@ -24,7 +24,7 @@ container_registry: | |||
base_url: docker.io | |||
# The owner or project of the registry | |||
# For example, "reedcompbio" if the image is available as docker.io/reedcompbio/allpairs | |||
owner: reedcompbio | |||
owner: pubtristanf |
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.
This will go once I undraft this PR.
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.
please update the file to have include: true for all that were turned to false
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.
Hi! Apologies for the late response on these reviews - this PR is a draft PR, so I didn't mean for anyone to do a deep review of its contents 😅
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's okay, I'm going through each PR in waiting.
scores_file, | ||
'-o', mapped_out_prefix + '/v-output.txt'] | ||
|
||
# Add optional arguments |
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.
These parameters are not currently in the example config.yaml file. Please add them as parameters a user can set.
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.
Is there a goal to allow for the other families other than the propagation family?
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.
Not at the moment.
|
||
@staticmethod | ||
def run(network=None, scores=None, output_file=None, delta=None, num_edges=None, density=None, | ||
time_limit=None, |
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.
Should we preset a time_limit?
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 wasn't quite sure how to handle that, as time_limit
feels like another option that should really be global to algorithms.
output_vertices.rename(output_file) | ||
|
||
@staticmethod | ||
def parse_output(raw_pathway_file, standardized_pathway_file): |
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.
Netmix2 will not work with the ML post processing and possibly some of the other post processing. I'm not sure how to fix this at the moment, but this is something that will need to be considered.
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.
Please add the expected-pathway file and input raw-pathway file
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.
Please add the expected input file
required_inputs = ['network', 'scores'] | ||
|
||
@staticmethod | ||
def generate_inputs(data: Dataset, filename_map): |
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.
For directionality and edges, I just wanted to confirm that NetMix2 deals with only undirected inputs and can handle duplicate edges?
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.
Unsure, I've only tested it on undirected inputs so far.
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.
Netmix itself could either interpret the network as fully directed, fully undirected, or mixed. From what I can tell it only deals with undirected graphs based on the code. It's not based on the universal input SPRAS/user is giving.
test/NetMix2/test_nm2.py
Outdated
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.
Please add test cases that includes the required parameters only, required + all optional parameters, missing any required parameters, and singularity test case.
WIP - need to actually handle gurobi from https://github.com/raphael-group/netmix2. Everything else is laid out.
Closes #157.