data.table is required but listed in `Suggests`

reichlab / zoltr

http://reichlab.io/zoltr/

GNU General Public License v3.0

2 stars 4 forks source link

data.table is required but listed in `Suggests` #38

Closed Bisaloo closed 2 years ago

Bisaloo commented 2 years ago

The data.table package is listed in Suggests but used unconditionally in the code. Since this is the only occurrence (outside of testing code), the easiest option is probably to use the base alternative:

  df <- as.data.frame(do.call(rbind, rows))

Bisaloo commented 2 years ago

I could open a new issue if you wish but on a related note, webmockr and mockery are listed in Imports when they seem to be used only in test code and should probably thus be under Suggests

matthewcornell commented 2 years ago

Thanks for the detailed report, Hugo. I will reply soon.

matthewcornell commented 2 years ago

Hugo, I tried your suggestions but they caused test warnings and check failures:

replacing the data.table line with yours caused three tests to fail, which (I'm not an R expert) tells me they're doing something different.
I got the error "checking package dependencies ... ERROR - Namespace dependencies not required: 'mockery', 'webmockr'", which might be coming from this line

My inclination is to leave the code as-is rather than spending more time on it, but I'm happy to hear your thoughts.

matthewcornell commented 2 years ago

Hugo, I tried your suggestions - thanks again - but they caused test warnings and check failures:

replacing the data.table line with yours caused three tests to fail, which (I'm not an R expert) tells me they're doing something different.
I got the error "checking package dependencies ... ERROR - Namespace dependencies not required: 'mockery', 'webmockr'", which might be coming from this line

My inclination is to leave the code as-is rather than spending more time on it, but I'm happy to hear your thoughts.

Bisaloo commented 2 years ago

I submitted a PR to change which packages are in Suggests and which ones are in Imports because it's an actual bug.

Regarding dropping the data.table dependency, it's indeed more complex than using

df <- as.data.frame(do.call(rbind, rows))

in this case because it's not a simple list but a list of lists. With do.call, we end up with list-columns instead of 'standard' columns. This can easily be fixed but it will come with a big performance hit where data.table is very efficient.

For this reason, I didn't change it for now but it might be worth revisiting at some point.