Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Issue176: fix filterFeatures() #177

Merged
merged 5 commits into from
Oct 13, 2022

Conversation

cvanderaa
Copy link
Collaborator

This PR should solve the issue highlighted in statOmics/msqrob2#42

There is also small code refactoring that will later be required for HDF5 support #171 (sorry for the leak).

@@ -309,6 +309,8 @@ filterFeaturesWithFormula <- function(object, filter, na.rm, keep, ...) {

## Internal function that
.checkFilterVariables <- function(object, vars) {
## Ignore variables from the user environment
vars <- vars[!vars %in% ls(envir = parent.frame(4))]
Copy link
Member

Choose a reason for hiding this comment

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

What is this number 4 here? Is it always 4?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it is always four because we only want to include variables from the environment the function was called in (may not always be .GlobalEnv). Here is a "traceback" counter:

0 in .checkFilterVariables()
1 in FilterFeaturesWithFormula()
2 in .local()
3 in filterFeatures()
4 in environment the function was called

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm improving the unit test to ensure this is checked.

Copy link
Member

Choose a reason for hiding this comment

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

Could you also add this information as a comment in the code.

@lgatto lgatto merged commit bed472c into rformassspectrometry:master Oct 13, 2022
@lgatto
Copy link
Member

lgatto commented Oct 13, 2022

Pushed to Bioc

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants