cjvanlissa / tidySEM

54 stars 7 forks source link

Some functions don't work if package is not fully loaded #93

Closed DominiqueMakowski closed 1 month ago

DominiqueMakowski commented 1 month ago

Currently if I run the graph_sem() function without loading the whole package using library() (which is needed when used as dependency in another package); we get the following:

> tidySEM::graph_sem(model)
Error in prepare_graph(model = model) : 
  could not find function "prepare_graph"
DominiqueMakowski commented 1 month ago

Additionally, I would suggest making explicit function calls package::fun() throughout the package to avoid masking issues. For instance, currently, loading ggraph after tidySEM breaks the functions due to similar function names

image

cjvanlissa commented 1 month ago

I'm looking into your first issue. The second issue is more complex, because the codebase is very large at the moment. Also, the screenshot you shared relates to exported functions from tidySEM having the same names as functions from these other packages, right? Not sure how that will be addressed by making explicit function calls within tidySEM?

cjvanlissa commented 1 month ago

Could you post a reproducible example or PR an integration test for your use case in the original post so I can verify that the recent commit fixes it?

DominiqueMakowski commented 1 month ago

Cheers, here are some reprexes:

First issue:

m <- lavaan::sem(' 
     ind60 =~ x1 + x2 + x3
     dem60 =~ y1 + a*y2 + b*y3 
', data = lavaan::PoliticalDemocracy)

tidySEM::graph_sem(m)
#> Registered S3 method overwritten by 'tidySEM':
#>   method          from  
#>   predict.MxModel OpenMx
#> Error in prepare_graph(model = m): could not find function "prepare_graph"

Created on 2024-05-22 with reprex v2.0.2

Second:

m <- lavaan::sem(' 
     ind60 =~ x1 + x2 + x3
     dem60 =~ y1 + a*y2 + b*y3 
', data = lavaan::PoliticalDemocracy)

library(tidySEM)
#> Warning: package 'tidySEM' was built under R version 4.3.3
#> Loading required package: OpenMx
#> Warning: package 'OpenMx' was built under R version 4.3.2
#> Registered S3 method overwritten by 'tidySEM':
#>   method          from  
#>   predict.MxModel OpenMx
library(ggraph)
#> Warning: package 'ggraph' was built under R version 4.3.2
#> Loading required package: ggplot2
#> Warning: package 'ggplot2' was built under R version 4.3.2
#> 
#> Attaching package: 'ggraph'
#> The following objects are masked from 'package:tidySEM':
#> 
#>     get_edges, get_nodes

tidySEM::graph_sem(m)
#> Error in (function (edges = NULL, layout = NULL, nodes = NULL, rect_width = 1.2, : Argument 'edges' must have columns 'from' and 'to'.

Created on 2024-05-22 with reprex v2.0.2

cjvanlissa commented 1 month ago

The first example still causes trouble after updating the package. The reason is that the default arguments of prepare_graph.lavaan() call several tidySEM functions, which are not available when the package is not loaded:

prepare_graph.lavaan <- function(model,
                                 edges = get_edges(x = model),
                                 layout = get_layout(x = model),
                                 nodes = get_nodes(x = model),
                                 ...){

My take is that this is fine because the function is clearly designed for interactive use; if the function will be called from within another package, these arguments should be specified explicitly.

What do you think?

cjvanlissa commented 1 month ago

For the second example: the problem with masking functions is already handled by base R:

#> Attaching package: 'ggraph'
#> The following objects are masked from 'package:tidySEM':
#> 
#>     get_edges, get_nodes

If you're proposing that I call tidySEM::get_edges() internally within the tidySEM package - that leads to CRAN check errors, so I don't think that's a good solution.

What do you think?

cjvanlissa commented 1 month ago

@DominiqueMakowski I removed the function calls from the default arguments. This should fix your issues; the reproducible examples you provided are now part of the integration tests.

DominiqueMakowski commented 1 month ago

Sorry for the late reply! Amazing, thanks a lot :)

cjvanlissa commented 4 weeks ago

On CRAN now, keep me updated!