rigraph
rigraph copied to clipboard
igraph
igraph::as_data_frame can overwrite dplyr::as_data_frame
Hi,
I just updated the new CRAN igraph version (1.0.1) and suddenly my code stopped working. The problem seems to be that in places where I called as_data_frame, the function igraph::as_data_frame instead of the desired dplyr::as_data_frame has now been called. The problem can be fixed by explicitly calling dplyr::as_data_frame, but that is a bit tedious.
Given that many users use dplyr, I wonder whether it would be a good idea to make igraph::as_data_frame compatible with dplyr. E.g. when the object is not a graph, don't throw an error but call dplyr::as_data_frame instead.
Best wishes, Sebastian
YEs, we could certainly make it generic. As for a workaround, you can do something like
as_data_frame <- dplyr::as_data_frame
and then your code will call dplyr.
If your code is in a package, then you can also selectively import functions from packages.
Great, thank!
Am 08.07.2015 um 09:39 schrieb Gábor Csárdi:
YEs, we could certainly make it generic. As for a workaround, you can do something like
as_data_frame <- dplyr::as_data_frame
and then your code will call |dplyr|.
If your code is in a package, then you can also selectively import functions from packages.
— Reply to this email directly or view it on GitHub https://github.com/igraph/rigraph/issues/95#issuecomment-119471672.
Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft. https://www.avast.com/antivirus
So I am thinking about fixing this, but I don't really see a way without involving dlyr authors. Calling dplyr is not an igraph object is really a hack.
What about changing the code in the beginning of igraph::as_data_frame e.g. as follows:
if (!is_igraph(x)) {
# if dplyr is loaded use its as_data_frame function
if ("package:dplyr" %in% search()) {
return(dplyr::as_data_frame(x))
# dplyr is not loaded. throw error
} else {
stop("Not a graph object")
}
}
If the type of x is not igraph, it just checks if dplyr is loaded and if yes calls its as_data_frame function. It doesn't use S3 dispatch, but I don't know whether that is neccessary.
Alternatively substitute _ with . in as_data_frame and just add S3 method for igraph objects to generic function as.data.frame from the base package... :)
The recent "underscore rampage" going through the R world has some downsides.
@skranz That's not ideal, because I would need to depend on dplyr, which I don't really want. Btw. I would need to still check the class of x, not just call dplyr.
@mbojan Yeah, as.data.frame would have been better. But then it looks different than the rest of the as_ functions in igraph.
Are you sure that you need to add dplyr in your dependencies with that construction?
if ("package:dplyr" %in% search()) {
return(dplyr::as_data_frame(x))
It is ensured that you only call dplyr when it is loaded in the user workspace, so you don't have to load dplyr in the igraph package. Of course, it might be that there is some CRAN test that fails with this construction... but I am not sure there is.
Concerning type checking: The line
if (!is_igraph(x)) {
still checks whether the objects is an igraph. If it is neither an igraph object nor an dpylr compatible object, you just can leave it to the dplyr function to throw the error message.
@skranz i agree with Gabor that it is a hack. I would definitely not want to add such hacks for all of the packages (out of ~7000 currently) that happened to include functions with the same names as mine. I think either as_data_frame should be a generic defined somewhere (base?) with igraph and dplyr providing methods (which requires quite some "organizational" effort), or stick with :: with some shortcuts ( like as_data_frame <- dplyr::as_data_frame) as suggested by @gaborcsardi earlier.
@gaborcsardi Sure. Perhaps as_-like names make their way to the R-core some day... BTW, do you plan to gradually deprecate "old" function names in igraph?
@mbojan, @gaborcsardi I agree that my solution is a hack, but maybe it is nevertheless worth to include it at least as a temporary solution. Am I right that as_data_frame was only just recently added to igraph? At least, some of my packages worked fine with earlier versions of igraph and the error only occured with the newest igraph version. If that is the case, there may be a lot of other package that suddenly break if the newest version of igraph package is installed. Then all those packages needed to be corrected with the as_data_frame <- dplyr::as_data_frame hack. Compared to this problem, I feel that temporary including the proposed hack into igraph is the smaller evil, at least until the issue is resolved in some other way (like making as_data_frame generic or renaming and depreciating the function).
@skranz Yes, I am sure about depending on dplyr.
With the number of packages growing, I think just using library() is asking for trouble. If you have a script, then consider using the as_data_frame <- dplyr::as_data_frame trick to select the functions you want to use, or use Stefan's import package: https://github.com/smbache/import If you are creating a package, then use importFrom to select specific functions.
As for the specific problem at hand, I need to think about it more, and maybe talk to Hadley. I remember this problem came up several times on the R-devel mailing list, so I just need to do some research about the possible solutions.
@mbojan For a while, the old names will stay. I have long term plans for breaking up igraph into smaller packages, and then the old names will only stay in the compatibility igraph package. At least these are my current plans.
I think the best solution is to rename igraph::as_data_frame to igraph::to_data_frame. Makes it more consistent with igraph::from_data_frame and also is a clue that the behavior is fundamentally different from dplyr::as_data_frame or as.data.frame, neither of which has a what argument.
More than five years have passed since the last comment here so I guess people have gotten used to as_data_frame() being present in at least two packages and learned how to pick the correct one. Closing the issue, but feel free to reopen if you think that this is still a problem.
