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

Decouple Ensembling #175

Merged
merged 9 commits into from
Sep 22, 2024
Merged

Conversation

ntalluri
Copy link
Collaborator

I got distracted and decoupled the ensembling code #163

@ntalluri
Copy link
Collaborator Author

ntalluri commented Aug 15, 2024

(Something to think about) Should I consider modularizing each part of the ML code? I can't recall if I tested each ML function individually with edge cases. There's a possibility that while HAC might work even though PCA fails when dealing with those edge cases. Right now the code will not run HAC after PCA fails due to the assumption that HAC will also fail.

spras/analysis/ml.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@agitter agitter left a comment

Choose a reason for hiding this comment

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

The overall design is good. I only have minor suggestions.

Should I consider modularizing each part of the ML code?

This is low priority for me given how many other things we have to work on. If a user ever requests that or has a problem with it, we could implement it.

Snakefile Show resolved Hide resolved
Snakefile Show resolved Hide resolved
Snakefile Show resolved Hide resolved
spras/analysis/ml.py Outdated Show resolved Hide resolved
test/ml/test_ml.py Outdated Show resolved Hide resolved
Snakefile Show resolved Hide resolved
spras/config.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@agitter agitter left a comment

Choose a reason for hiding this comment

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

I don't have any major comments remaining. I'll test locally and merge if everything looks good.

spras/config.py Outdated Show resolved Hide resolved
Snakefile Show resolved Hide resolved
@agitter
Copy link
Collaborator

agitter commented Sep 12, 2024

The tests passed locally. However, when I ran with the config file, I was confused about the the data1 outputs. Should we have ML outputs (e.g. PCA output) for Omics Integrator 1? We run it with 4 parameter combinations and the summary table shows:

output/data1-omicsintegrator1-params-E3LSEZQ/pathway.txt	3	2	1	2	2	1	1
output/data1-omicsintegrator1-params-NFIPHUX/pathway.txt	0	0	0	0	0	0	0
output/data1-omicsintegrator1-params-SU2S63Y/pathway.txt	3	2	1	2	2	1	1
output/data1-omicsintegrator1-params-V26JBGX/pathway.txt	0	0	0	0	0	0	0

I didn't get those outputs.

@ntalluri
Copy link
Collaborator Author

ntalluri commented Sep 17, 2024

When I run OI1 locally, I am getting back outputs for the ml step. I keep rerunning oi1 for the config file and seem to be getting back what I expect.

@agitter
Copy link
Collaborator

agitter commented Sep 22, 2024

When I ran it, I thought I did not get those OI1 ML outputs. However, I tested it again and I did. I could have been mistaken or had some temporary error.

@agitter agitter merged commit 624cbb4 into Reed-CompBio:master Sep 22, 2024
5 checks passed
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.

3 participants