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

Filter: Improve method of loading required filter pkgs #2483

Merged
merged 26 commits into from Jan 31, 2019

Conversation

4 participants
@pat-s
Copy link
Member

pat-s commented Nov 13, 2018

As @mb706 pointed out, this should be handled (for filters) in

if (!(any(sapply(filter, function(x) !isScalarNA(filter$pkg)))))
lapply(filter, function(x) requirePackages(x$pkg, why = "generateFilterValuesData", default.method = "load"))

However,issue #2484 applies.

This PR solves the issue by

  • setting the pkg slot of a filter that requires no pkg to be loaded to NULL.
  • changing the if statement from above so that it actually works
@mb706

This comment has been minimized.

Copy link
Contributor

mb706 commented Nov 13, 2018

Doesn't the pkg = "FSelectorRcpp", argument in the makeFilter call already cause the package to be loaded when the filter is being used?

@berndbischl

This comment has been minimized.

Copy link
Contributor

berndbischl commented Nov 13, 2018

I don't like this custom change. We should load packages always in the same way

@pat-s

This comment has been minimized.

Copy link
Member Author

pat-s commented Nov 13, 2018

@mb706 I need to check this.

@berndbischl we already use this way of loading the NS in the pkg. On the phone atm so can't refer to an example.

Just discovered today that all these msg are really annoying during usage.

@berndbischl

This comment has been minimized.

Copy link
Contributor

berndbischl commented Nov 13, 2018

Ok that's new to me? Please link to code

@berndbischl

This comment has been minimized.

Copy link
Contributor

berndbischl commented Nov 13, 2018

Actually please explain the problem first. That should always be done for a PR...

@mb706

This comment has been minimized.

Copy link
Contributor

mb706 commented Nov 13, 2018

See #2484, apparently requirePackages should be called here, but isn't.

@pat-s

This comment has been minimized.

Copy link
Member Author

pat-s commented Nov 14, 2018

Ok that's new to me? Please link to code

Idk, just "searched in files" the whole repo but couldn't find any instances.
Where did I get that from? Can't tell right now... 🤦‍♂️

Ok, so we should use BBmisc::requirePackages(). But @mb706 pointed to a deeper lying problem here that we should tackle that makes the requirePackages() calls in the FSelectorRcpp implementation obsolete because NS should be loaded in the generateFeatureValuesData() call.

@pat-s

This comment has been minimized.

Copy link
Member Author

pat-s commented Nov 14, 2018

Take a look at #2484 (comment).

@pat-s pat-s force-pushed the silent-namespace branch from 9bc8b9e to a1979c4 Nov 14, 2018

@pat-s

This comment has been minimized.

Copy link
Member Author

pat-s commented Nov 14, 2018

Not yet finished/working. Will continue after the meeting.

@mb706

This comment has been minimized.

Copy link
Contributor

mb706 commented Nov 14, 2018

The assertCharacter(pkg... check is good as it was imo; since a filter can depend on multiple packages it makes sense to use character(0) for the "empty set". Changing that to NULL is needlessly changing stuff around. Also, since a user can define custom filters relying on other packages, the set of possible packages should not be enforced.

The real problem is the if clause. it should probably be replaced by something like

allpkg = unlist(extractSubList(filter, "pkg"))
requirePackages(allpkg, why = "generateFilterValuesData", default.method = "load")

(sorry am on mobile rn so can't test. maybe one needs an if (length(allpkg)) somewhere in there). All other changes should probably be undone.

pat-s added some commits Nov 14, 2018

@pat-s

This comment has been minimized.

Copy link
Member Author

pat-s commented Nov 14, 2018

Also, since a user can define custom filters relying on other packages, the set of possible packages should not be enforced.

That's a good point. I changed it back to assert_character(null.ok = TRUE).

@pat-s pat-s changed the title Filter: require Namespace silently Filter: Improve method of loading required filter pkgs Nov 14, 2018

pat-s added some commits Nov 14, 2018

@berndbischl

This comment has been minimized.

Copy link
Contributor

berndbischl commented Nov 15, 2018

I don't get the change. See Martins last comment. Why should packages be Null? And why should Nas be allowed? Packages should be a char vec, without Nas. If empty no packages are required. If not empty all packs are required

@pat-s

This comment has been minimized.

Copy link
Member Author

pat-s commented Nov 15, 2018

Why should packages be Null? And why should Nas be allowed?

Packages are still a chr vec. The only thing that changed is that filters that require NO pkg to be loaded (i.e. these pkgs are in base-R and loaded by default) now do not have a slot pkg = character(0) but pkg = NULL instead.

That's why I changed the assert function to accept NULL. NULL is imo more clean than using character(0) to me.
If we do only want to accept chrs, we should explicitly load the respective pkgs even if they are already loaded, e.g. for filter variance the stats pkg.

The param decription of pkg in makeFilter() is: "Source package where the filter is implemented."
To me it does not make sense why this should evaluate to character(0) in any case?

pat-s added some commits Nov 15, 2018

R/Filter.R Outdated
if (perf.measure[[1L]]$minimize)
res = -1.0 * res
setNames(res, fns)
if (is.null(perf.measure))

This comment has been minimized.

@mb706

mb706 Nov 19, 2018

Contributor

Changing indentation here is probably going to be misleading.

This comment has been minimized.

@pat-s

pat-s Nov 20, 2018

Author Member

Thanks, done. Added brackets so the auto-indent works properly.

pat-s added some commits Nov 20, 2018

@pat-s

This comment has been minimized.

Copy link
Member Author

pat-s commented Nov 20, 2018

@berndbischl

This comment has been minimized.

Copy link
Contributor

berndbischl commented Nov 20, 2018

Packages are still a chr vec. The only thing that changed is that filters that require NO pkg to be loaded (i.e. these pkgs are in base-R and loaded by default) now do not have a slot pkg = character(0) but pkg = NULL instead.

i still dont get why we do that? as this is not consistent with what we do in learners?
character(0) should be used if no packages are supposed to be loaded?

pat-s added some commits Nov 22, 2018

@pat-s

This comment has been minimized.

Copy link
Member Author

pat-s commented Nov 22, 2018

i still dont get why we do that?

I found it more logic to set a not-existend value to NULL rather than character(0). But if we introduce inconsistency with that, fair enough :)

@berndbischl tidied up everything, ready-to-go now?

pat-s added some commits Nov 22, 2018

@pat-s pat-s force-pushed the silent-namespace branch from 1e26301 to 6f04995 Nov 25, 2018

@pat-s pat-s force-pushed the silent-namespace branch from 6f04995 to f6a7ade Nov 25, 2018

@pat-s

This comment has been minimized.

Copy link
Member Author

pat-s commented Nov 26, 2018

pat-s and others added some commits Jan 31, 2019

@pat-s pat-s merged commit 5c4a48c into master Jan 31, 2019

1 check passed

deploy/netlify Deploy preview ready!
Details

@pat-s pat-s deleted the silent-namespace branch Jan 31, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment