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

adding css method #614

Merged
merged 32 commits into from
Sep 23, 2024
Merged

Conversation

SHillman836
Copy link
Contributor

No description provided.

R/transformCounts.R Outdated Show resolved Hide resolved
tests/testthat/test-5transformCounts.R Outdated Show resolved Hide resolved
R/transformCounts.R Outdated Show resolved Hide resolved
R/transformCounts.R Outdated Show resolved Hide resolved
R/transformCounts.R Outdated Show resolved Hide resolved
R/transformCounts.R Outdated Show resolved Hide resolved
R/transformCounts.R Outdated Show resolved Hide resolved
Copy link
Member

@antagomir antagomir left a comment

Choose a reason for hiding this comment

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

Seems good. Just minor comments

R/transformCounts.R Outdated Show resolved Hide resolved
R/transformCounts.R Outdated Show resolved Hide resolved
man/transformAssay.Rd Outdated Show resolved Hide resolved
DESCRIPTION Outdated Show resolved Hide resolved
R/transformCounts.R Outdated Show resolved Hide resolved
R/transformCounts.R Outdated Show resolved Hide resolved
R/transformCounts.R Outdated Show resolved Hide resolved
R/transformCounts.R Outdated Show resolved Hide resolved
R/transformCounts.R Outdated Show resolved Hide resolved
R/transformCounts.R Outdated Show resolved Hide resolved
@TuomasBorman
Copy link
Contributor

Couple points from .calc_css_percentile function (I think that I fixed most of them already). The functions looked fine and it worked well, but there was room for improvement

  1. Some of the indentations were incorrect. Should be multiplication of 4 spaces
  2. Some lines were too long. Should be max 80 characters
  3. Avoid loops. Vectorize if possible (faster than loops, and also good practice)
  4. Instead of sapply, use vapply. (Comes from Bioconductor recommendations)
  5. Instead of 1:10, use seq_len(10). (Comes also from Bioc)

@antagomir
Copy link
Member

@Daenarys8 could you add the suggested updates and make a PR against the SHillman836::adding-css-method branch? Or can @SHillman836 help to finalize this one?

@Daenarys8 Daenarys8 changed the base branch from devel to adding-css-method August 28, 2024 05:50
Signed-off-by: Daena Rys <rysdaena8@gmail.com>
@Daenarys8 Daenarys8 changed the base branch from adding-css-method to devel August 28, 2024 08:16
Signed-off-by: Daena Rys <rysdaena8@gmail.com>
R/transformCounts.R Outdated Show resolved Hide resolved
R/transformCounts.R Outdated Show resolved Hide resolved
R/transformCounts.R Outdated Show resolved Hide resolved
R/transformCounts.R Outdated Show resolved Hide resolved
@TuomasBorman
Copy link
Contributor

It seems that metagenomeSeq is only used in testing? We can probably just hard-code those values to testing file. That way we do not need this new dependency.

@Daenarys8
Copy link
Collaborator

we can ask them?

Sure. I found an existing issue from March this year and updated it. It seems the package had been inactive for a while.

Signed-off-by: Daena Rys <rysdaena8@gmail.com>
R/transformCounts.R Outdated Show resolved Hide resolved
Copy link

codecov bot commented Sep 2, 2024

Codecov Report

Attention: Patch coverage is 83.09859% with 12 lines in your changes missing coverage. Please review.

Please upload report for BASE (devel@3f3b968). Learn more about missing BASE report.

Files with missing lines Patch % Lines
R/transformCounts.R 82.85% 12 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             devel     #614   +/-   ##
========================================
  Coverage         ?   67.83%           
========================================
  Files            ?       44           
  Lines            ?     5419           
  Branches         ?        0           
========================================
  Hits             ?     3676           
  Misses           ?     1743           
  Partials         ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Daena Rys <rysdaena8@gmail.com>
Signed-off-by: Daena Rys <rysdaena8@gmail.com>
R/transformCounts.R Outdated Show resolved Hide resolved
R/transformCounts.R Outdated Show resolved Hide resolved
@Daenarys8
Copy link
Collaborator

It seems css and css_fast is already in mia?
@TuomasBorman

Sync

# Conflicts:
#	R/transformCounts.R
#	man/transformAssay.Rd
@TuomasBorman
Copy link
Contributor

It seems css and css_fast is already in mia? @TuomasBorman

It did not include the functionality; it included only "css" as one of the options. I am not sure how that ended up there, probably I copy-pasted the method-vector there incorrectly

@TuomasBorman TuomasBorman merged commit 3a3c78b into microbiome:devel Sep 23, 2024
2 of 3 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.

4 participants