-
Notifications
You must be signed in to change notification settings - Fork 8
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
Two-step hybrid refactoring #34
Two-step hybrid refactoring #34
Conversation
maximskorik
commented
Apr 20, 2022
•
edited
Loading
edited
- added test data for two-step hybrid
- switched most of data handling functionality to dplyr framework
- refactored two-step hybrid into individual steps
- extracted repeating methods in adjust.time.R and feature.align.R
- reformated semi.sup.R and two.step.hybrid.R
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.
Amazing work! Couple small changes to fix some possible sources of error and this can be merged!
I'll run some additional tests manually later as well and see if everything runs smoothly.
R/two.step.hybrid.R
Outdated
for (batch_id in batches_idx) | ||
{ | ||
this.fake <- step_one_features[[batch_id]] | ||
this.fake.medians <- distinct(this.fake, mz, rt, intensity)$intensity |
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.
Are these really the medians? If so, how do the distinct intensities form the medians?
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.
At this step this.fake
is a long table where intensity is the column name of median intensities. Since each feature is repeated in multiple rows (a row for a sample) and values in intensity column is the same for all samples of a given feature, distinct
selects a median intensity of each unique feature.
In retrospect, calling a column that stores median intensities as intensity is confusing. Fixed that in 976e416.
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.
cool - how comes that the distinct(...)
function actually computes the median intensities? I thought that it actually selects the unique values in the list?
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 doesn't compute the medians. The medians are already computed and stored in the intensity column. distinct(...)
only selects the median intensities of each unique feature.
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.
Refactored this part in 99ae4f5.
R/two.step.hybrid.R
Outdated
colnames(final.ftrs) <- stringr::str_remove_all(colnames(final.ftrs), ".mzml") | ||
colnames(final.times) <- stringr::str_remove_all(colnames(final.times), ".mzml") |
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 this required, to strip the file ending? If so, this should be done more robustly.
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, it's not required. Having a file extension appended to a sample name in the table just doesn't seem pleasant or valuable.
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.
Then this should be changed to strip any file ending since there could also be other names than only ".mzml"
.
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.
Done in 4e89fa3.
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.
Looks good!
colnames(final.ftrs)[sample_cols_idx] <- tools::file_path_sans_ext(colnames(final.ftrs)[sample_cols_idx]) | ||
colnames(final.times)[sample_cols_idx] <- tools::file_path_sans_ext(colnames(final.times)[sample_cols_idx]) |
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.
Nice way to exclude the file endings!