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

Use proper sample IDs inside feature tables #153

Merged
merged 14 commits into from
Oct 25, 2022

Conversation

xtrojak
Copy link

@xtrojak xtrojak commented Oct 21, 2022

All feature tables contained arbitrary (resp. based on input order) integer sample IDs. The original name of the sample was lost during the feature extraction process. This PR reintroduces them again and uses them to identify the sample across the computation.

Requires files update here.

R/utils.R Outdated Show resolved Hide resolved
R/compute_clusters.R Outdated Show resolved Hide resolved
R/feature.align.R Outdated Show resolved Hide resolved
@maximskorik
Copy link
Member

maximskorik commented Oct 24, 2022

@xtrojak, I've checked out to this branch and fetched the new test data but the following tests fail: adjust_time, compute_clusters, feature-align, hybrid, recover-weaker, unsupervised. Some of them due to actual output not being equal to expected and some (recover-weaker and adjust-time) with this error message:

...
Can't combine `..1$sample_id` <double> and `..2$sample_id` <character>.

or

...
Can't combine `..1$sample_id` <character> and `..2$sample_id` <integer>.

Do you have any idea what I might be missing? I've run the tests in the devcontainer.

@maximskorik
Copy link
Member

maximskorik commented Oct 24, 2022

The same tests fail with devtools::check('.', error_on = 'error'):

[ FAIL 14 | WARN 21 | SKIP 4 | PASS 24 ]
Error: Test failures
In addition: Warning messages:
1: In features$cluster : closing unused connection 6 (<-localhost:11634)
2: In features$cluster : closing unused connection 5 (<-localhost:11634)
Execution halted

1 error ✖ | 6 warnings ✖ | 7 notes ✖
Error: R CMD check found ERRORs
Execution halted

@xtrojak
Copy link
Author

xtrojak commented Oct 25, 2022

@maximskorik Are you sure you fetched the new test data properly? If you just run the wget commands, the files are not overwritten but a new copy is placed alongside them (with a .1, .2 ... suffix).

I suppose we should add -O parameter to the readme.

@maximskorik
Copy link
Member

@maximskorik Are you sure you fetched the new test data properly? If you just run the wget commands, the files are not overwritten but a new copy is placed alongside them (with a .1, .2 ... suffix).

I suppose we should add -O parameter to the readme.

Thanks for the advice! That must've been my problem.

Copy link
Member

@maximskorik maximskorik left a comment

Choose a reason for hiding this comment

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

All tests pass and the new test-data looks good. Thanks, @xtrojak!
This can be merged. Also if you could update the call to wget with -O in README.md that would be nice.

@hechth hechth merged commit 5ac72f5 into RECETOX:master Oct 25, 2022
@hechth hechth linked an issue Nov 1, 2022 that may be closed by this pull request
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.

Correct sample name is not contained in output
3 participants