stan-dev / projpred

Projection predictive variable selection
https://mc-stan.org/projpred/
Other
109 stars 25 forks source link

Incompatability with R version <4.2.0 #423

Closed l-gorman closed 1 year ago

l-gorman commented 1 year ago

Summary

The CV varsel function uses an argument which is new to R 4.2.0, see here. This led to an error when running this reprex.

Issue discussed in more detail on the Stan Forum.

I have forked the repo here. I will test the changes on the Reprex and see if it works. If so would you mind if I open a PR? Feel free to say no, as it is a very minor change!!!

Apologies I could not run the test suite, every time I try to run it my R session aborts!

Thanks again for all the great work with projpred, and all the support!!!

fweber144 commented 1 year ago

Thank you for reporting this. I probably introduced this bug in #419 (more precisely, commit e11edc75e330be205a6a28b619eaeb4f11b7a2f7) where I started to use simplify2array() and its except argument. (Unfortunately, the documentation for simplify2array() doesn't mention that except is only available in versions >= 4.2.0, so I wasn't aware of it. Besides, CRAN only requires to test with the most recent "old release" of R which at the time of the CRAN release of projpred 2.6.0 was 4.2.3, so it also didn't get caught in the unit tests.)

Unfortunately, except cannot simply be removed (as you do in #424) because it is needed for the case where a forward search is run with nterms_max = 0 (which should not occur in practice, but still, this is a supported case, so we need to support it further). But nevertheless, thanks for creating a PR! This is much appreciated. For a possible solution, I will continue in #424.

fweber144 commented 1 year ago

Apologies I could not run the test suite, every time I try to run it my R session aborts!

If you have time, would you mind creating a new issue describing what exactly happens when you try to run the unit tests?

l-gorman commented 1 year ago

Unfortunately, except cannot simply be removed (as you do in https://github.com/stan-dev/projpred/pull/424) because it is needed for the case where a forward search is run with nterms_max = 0 (which should not occur in practice, but still, this is a supported case, so we need to support it further). But nevertheless, thanks for creating a PR! This is much appreciated. For a possible solution, I will continue in https://github.com/stan-dev/projpred/pull/424.

I understand, I thought there must be a reason you included that argument, and I thought that my solution would be too simple to work! In that case, I think I will just use my own fork for the analysis, as I cannot get the HPC team to add the most recent version of R just for my analysis! I will close the PR!

If you have time, would you mind creating a new issue describing what exactly happens when you try to run the unit tests?

No problem will do!

fweber144 commented 1 year ago

In that case, I think I will just use my own fork for the analysis, as I cannot get the HPC team to add the most recent version of R just for my analysis!

Thank you, then I will simply set the minimum required R version to 4.2.0. (I reopened to close this when I managed to do so.)

l-gorman commented 1 year ago

Great 🙂, apologies for closing it prematurely then!

fweber144 commented 1 year ago

No problem :)

fweber144 commented 1 year ago

I decided to fix the issue underlying the incompatibility instead of requiring R >= 4.2.0, see #427. (In the future, if there is something else requiring R >= 4.2.0, then we can remove the special case I introduced in #427. But requiring R >= 4.2.0 only for this would seem a bit too strict, especially because we have a fix readily available.)

Now I have a last question to you, @l-gorman: Could you try the reprex from https://github.com/stan-dev/projpred/pull/427#issue-1776377513 on your machine / cluster with R < 4.2.0? Afterwards, I would merge #427.

l-gorman commented 1 year ago

Can confirm that this works!

fweber144 commented 1 year ago

Thanks!