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

Implemented new version for clustering #201

Merged
merged 36 commits into from
May 3, 2023
Merged

Conversation

hechth
Copy link
Member

@hechth hechth commented Apr 25, 2023

  • Implemented new simple clustering computation method based on only numerical tolerances and using dplyr operations.
  • Implemented unit tests for new clustering method
  • Implemented unit tests for find_mz_tolerance
  • Implemented unit test for find.tol.time

Closes #198
Closes #199

Copy link

@xtrojak xtrojak left a comment

Choose a reason for hiding this comment

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

@hechth Why did you remove all the .Rd files in cec9f8a?

@hechth
Copy link
Member Author

hechth commented Apr 28, 2023

@hechth Why did you remove all the .Rd files in cec9f8a?

They are auto generated anyway, so I think they just increase the repository size unnecessarily

@xtrojak
Copy link

xtrojak commented Apr 29, 2023

@hechth Why did you remove all the .Rd files in cec9f8a?

They are auto generated anyway, so I think they just increase the repository size unnecessarily

I thought they are maybe needed somewhere, but I guess you are right. Would be nice to note this in the changelog.

Copy link

@xtrojak xtrojak left a comment

Choose a reason for hiding this comment

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

Good job with the rewrite of clustering, looks awesome!

R/compute_clusters.R Outdated Show resolved Hide resolved
tests/testthat/test-find.tol.time.R Outdated Show resolved Hide resolved
Copy link

@zargham-ahmad zargham-ahmad left a comment

Choose a reason for hiding this comment

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

Looks good!

R/compute_clusters.R Outdated Show resolved Hide resolved
@@ -0,0 +1,21 @@
test_that("compute_rt_tol_relative computes something", {

Choose a reason for hiding this comment

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

"computes something"?

@zargham-ahmad
Copy link

@hechth Does deleting man files really make any difference in the repo size because they were around 80 kb anyways? Also, I think the CI seems to be failing because of it No man pages found in package ‘recetox.aplcms’

@codecov
Copy link

codecov bot commented May 3, 2023

Codecov Report

Patch coverage: 97.34% and project coverage change: -0.29 ⚠️

Comparison is base (0543377) 66.13% compared to head (d840dd7) 65.84%.

❗ Current head d840dd7 differs from pull request most recent head b5a61ce. Consider uploading reports for the commit b5a61ce to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #201      +/-   ##
==========================================
- Coverage   66.13%   65.84%   -0.29%     
==========================================
  Files          23       23              
  Lines        2616     2685      +69     
==========================================
+ Hits         1730     1768      +38     
- Misses        886      917      +31     
Impacted Files Coverage Δ
R/adjust.time.R 63.63% <ø> (ø)
R/find.tol.time.R 48.00% <ø> (-42.00%) ⬇️
R/prof.to.features.R 87.19% <0.00%> (ø)
R/remove_noise.R 58.33% <ø> (ø)
R/compute_clusters.R 89.15% <100.00%> (+7.52%) ⬆️
R/feature.align.R 96.38% <100.00%> (ø)
R/find_mz_tolerance.R 96.77% <100.00%> (ø)
R/hybrid.R 100.00% <100.00%> (ø)
R/recover.weaker.R 98.32% <100.00%> (ø)
R/unsupervised.R 99.29% <100.00%> (ø)
... and 1 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Co-authored-by: Zargham Ahmad <46793118+zargham-ahmad@users.noreply.github.com>
@hechth hechth merged commit c81061b into RECETOX:master May 3, 2023
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.

update test data for extract-features full data test case implement new way of clustering
3 participants