search

nfj1380 / mrIML

Multivariate (multi-response) ensemble learning
https://nfj1380.github.io/mrIML/
Other
6 stars 5 forks source link

Unit tests #3

Closed timcdlucas closed 3 years ago

timcdlucas commented 3 years ago

Hi,

I was reviewer 3 on your paper submission. I've now been sent your resubmission for re-review. So first let me say congratulations on all the great work you've done since.

I thought I'd message you directly here about unit tests rather than via the editors. I hope that's ok. Everything except the unit test comment looks great, so I'm hoping I'll be able to just say "the authors have responded to all my queries, recommend accept" in my rereview.

However, I can't currently see any unit tests in this repo. I can see that you've set up CI and run R CMD check on the package (which is great). And there's possibly temp_pipeline.R which runs functions but doesn't test them (and it doesn't look like that script gets run by the github actions).

However, it may well just be that I've missed something somewhere or something like that. testthat is in the sessionInfo() in the README for example. The tests folder is stated in .gitignore as a folder to be ignored. So maybe there are tests and they just haven't been committed by accident.

So, if I have missed the tests could you possibly point me to where they are? To be completely explicit, by unit tests I mean code that runs functions and compares the output with prespecified expected outputs. As described here for example https://r-pkgs.org/tests.html

Again, I hope you're ok with me approaching you outside the review. Just, everything goes so slowly through editors that it'd be a total shame if I said "there's still no tests" and a month later I get an email back from you, via the editors saying "yes, it's just in some other folder".

Tim

gustavo-etal commented 3 years ago

@timcdlucas Thank you very much for asking it over here. We have created the use_testthat(), I am adding +2 new functions with a new website. I will post push it shortly.

timcdlucas commented 3 years ago

Hi @gustavo-etal. That's great news. I think I'm now late on my review so I think I'll just turn in the review and say you've responded to all my comments. And then I guess hopefully we'll just have an honourable agreement that you will get these unit tests pushed asap!

CNuge commented 3 years ago

Hello,

I have been exploring this package after reading the paper as it seems to be of potential use in my research. I also noticed that there were no unit tests associated with the repository, and am therefore unsure how to validate if the package does what it says it does. I was about to open a separate issue, but saw that this problem had been previously raised in the thread above. Therefore, I'm just wondering if there are any plans for validation of the code with unit tests in the near future?

Additionally (as opposed to opening an additional issue) I tried to run through the vignettes to get oriented to the package, but all three vignettes appear to contain errors that prevent the examples from being run front -> back without going in and performing manual edits to the vignette files. The vignettes all throw similar errors when calling the MrIML functions, where the arguments in the vignettes appear to no match the arguments the functions accept.

i.e. in the file "Vignette_regression.rmd" just running the cells sequentially fails at the line: ModelPerf <- mrIMLperformance(yhats, model1, X=X, model='regression') which throws the error: Error in mrIMLperformance(yhats, model1, X = X, model = "regression") : unused argument (X = X)

Thanks.