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

Phili #79

Merged
merged 16 commits into from
Jan 19, 2024
Merged

Phili #79

merged 16 commits into from
Jan 19, 2024

Conversation

philouail
Copy link
Collaborator

Here is the PR related to the issue #77.

These basic functions return single values and vectors (numeric for all expect for rowBlank which is logical).

This is a first draft, input is welcome, specifically on:

  • naming of functions: for example should this really be called filteringFunctions ? as they don't really filter but more compute measurement of precision and detection / quality measurement of our data. I think a name reflecting that would be more accurate. qualityFunctions ?
  • rowBlank threshold

Also, @jorainer I took your description and code for the rsd and rowRsd, it therefore needs to be removed from CompMetaboTools if you are fine with that.

I am currently writing the vignette but wanted to check if that was what you had in mind for now.
Also next I will implement more user friendly function that actually filter the data.

@philouail philouail added the enhancement New feature or request label Jan 10, 2024
Copy link
Member

@jorainer jorainer left a comment

Choose a reason for hiding this comment

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

Thanks for the great contribution @philouail ! I have some minor change requests.

R/function-filtering.R Outdated Show resolved Hide resolved
R/function-filtering.R Outdated Show resolved Hide resolved
R/function-filtering.R Outdated Show resolved Hide resolved
R/function-filtering.R Outdated Show resolved Hide resolved
R/function-filtering.R Outdated Show resolved Hide resolved
R/function-filtering.R Outdated Show resolved Hide resolved
R/function-filtering.R Outdated Show resolved Hide resolved
tests/testthat/test_function-filtering.R Show resolved Hide resolved
@philouail
Copy link
Collaborator Author

Thanks for the comments I fixed them, do you have an opinion on the naming thing I was talking about ? replace filteringFunctions with some kind of name that include quality instead ? as these functions don't filter.

I added a section to the vignettes to describe general use of these functions.

Copy link
Member

@jorainer jorainer left a comment

Choose a reason for hiding this comment

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

Great contribution @philouail ! Also very clear description of the functionality in the vignette!

Also, I agree that "filtering" might not be the right term here - I would maybe do some re-formulations (talking about quality assessment and less of filtering) and also rename the R file accordingly.

R/function-filtering.R Outdated Show resolved Hide resolved
R/function-filtering.R Outdated Show resolved Hide resolved
R/function-filtering.R Outdated Show resolved Hide resolved
vignettes/MetaboCoreUtils.Rmd Outdated Show resolved Hide resolved
vignettes/MetaboCoreUtils.Rmd Outdated Show resolved Hide resolved
vignettes/MetaboCoreUtils.Rmd Outdated Show resolved Hide resolved
vignettes/MetaboCoreUtils.Rmd Outdated Show resolved Hide resolved
@philouail
Copy link
Collaborator Author

The naming system is now fixed !

@michaelwitting what do you think ? any ideas for improvement ?

Copy link
Collaborator

@michaelwitting michaelwitting 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 to me.

@jorainer jorainer merged commit bb3d08a into main Jan 19, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants