ggobi / tourr

A implementation of tour algorithms in R
http://ggobi.github.io/tourr/
Other
65 stars 21 forks source link

Option tourr.verbose #100

Closed casperhart closed 1 year ago

casperhart commented 3 years ago

Hi,

I'm currently using tourr programatically, and I've run in to some issues to do with logging and verbosity. What I'm doing is something like below

library(tourr)
tour = new_tour(flea[, 1:6], tour_path = grand_tour())
start=tour(0)
step_size = 1/10
projections = vector("list", 10)
for (i in 1:100) {
  projections[[i]] = tour(step_size)$proj
}

However, this code will fail with the following error after a few iterations:

Error in if ("new_basis" %in% rcd_env[["record"]]$info & rcd_env[["record"]]$method[2] !=  : 
  argument is of length zero

This is because in the file tour.R there is a call to parent.frame() that looks for an object called record in the parent environment, which doesn't exist.

Creating a data frame called record with the appropriate columns before running the loop above fixes this issue, but it will still fail if I try to use lapply() or purrr::map() instead of a for loop.

I see from the git history there was an option called tour.verbose , but this was removed in #96. I'd like to reinstate something similar---either an option or a function argument---to prevent the need for the record data frame and to suppress output to the console.

Is there a preferred way to implement this? I'm happy to contribute.

earowang commented 3 years ago

Perhaps you can send a PR first? and specify ggobi/tourr#PR in the Remotes of {d3tourr} DESCRIPTION, so we can use the fixed version?

earowang commented 3 years ago

Had the same issue with @sa-lee {liminal} https://github.com/sa-lee/liminal

casperhart commented 3 years ago

Before making a PR, I wanted to know if there was a reason the tourr.verbose option was removed, otherwise reimplementing it may be wasted effort. And is using options the best way to do this, or should it be implemented as a function argument?

huizezhang-sherry commented 3 years ago

In an even older version, verbose was implemented as a function argument, however, ith both designs, initialising a record object will introduce <<- when appending new records, which causes a problem with R CMD CHECK.

parent.frame() is introduced to get away with using <<-.

huizezhang-sherry commented 3 years ago

If the purpose of your initial codes is to extract projection matrices, rather than viewing the tour animation, I think the best way to do that is through save_history() and interpolate():

library(tourr)
t1 <- save_history(flea[, 1:6],max = 10)
i1 <- interpolate(t1, 1/10)
sa-lee commented 3 years ago

just chiming into say this is still an issue for me tool with the latest version because of parent.frame(), did you manage to solve this @casperhart?

@huizezhang-sherry that isn't a solution in our case as we want to generate the tour sequence dynamically rather than using a fixed number of max bases. @casperhart and @earowang are correct in saying that parent.frame() is the culprit here. I will take a look into fixing today. I think this could be solved by explicitly passing an environment that contains the record object.

huizezhang-sherry commented 3 years ago

I'm happy with the solution of passing an environment.

sa-lee commented 3 years ago

ok did you want to have a go of refactoring or do you want me to put in a PR sometime over the next week?

huizezhang-sherry commented 3 years ago

I can have a try first and let you know how it goes

sa-lee commented 3 years ago

Great, thanks!

casperhart commented 3 years ago

If the purpose of your initial codes is to extract projection matrices, rather than viewing the tour animation, I think the best way to do that is through save_history() and interpolate():

library(tourr)
t1 <- save_history(flea[, 1:6],max = 10)
i1 <- interpolate(t1, 1/10)

@huizezhang-sherry This should solve the problem I'm having, thanks. Feel free to close the issue once @sa-lee 's issue is resolved

huizezhang-sherry commented 3 years ago

@sa-lee do you have an example of the issue you have with the current version?

sa-lee commented 3 years ago

The example @casperhart made is sufficient to show the issue I'm having.