stan-dev / projpred

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

Fix pareto warnings #498

Closed avehtari closed 1 month ago

avehtari commented 3 months ago

Fixes #490

fweber144 commented 2 months ago

Similarly to #496, I have added some comments, but I'm not done with the review yet.

Besides, I think documentation needs to be updated (at least re-roxygenized) and I haven't run R CMD check (including the unit tests) yet. The NEWS file could perhaps also be updated.

avehtari commented 2 months ago

fixed ::: and ref, devtools::check() reported

❯ checking compiled code ... OK
   WARNING
  ‘qpdf’ is needed for checks on size reduction of PDFs

❯ checking package dependencies ... NOTE
  Packages suggested but not available for checking:
    'optimx', 'unix', 'glmnet', 'future.callr'

❯ checking Rd cross-references ... NOTE
  Package unavailable to check Rd xrefs: ‘unix’

❯ checking compilation flags used ... NOTE
  Compilation used the following non-portable flag(s):
    ‘-march=native’

0 errors ✔ | 1 warning ✖ | 3 notes ✖
fweber144 commented 2 months ago

Commit 930ad81 is purely cosmetic. Feel free to revert it if you don't like it.

avehtari commented 2 months ago

I'm getting these errors when running tests

Failure (]8;line = 225:col = 7;file:///u/77/ave/unix/proj/projpred/tests/testthat/test_as_matrix.Rtest_as_matrix.R:225:7]8;;): as.matrix.projection() works
Snapshot of code has changed:
    old                                      | new                                         
[5] Code                                     | Code                                     [5]
[6]   print(rlang::hash(m))                  |   print(rlang::hash(m))                  [6]
[7] Output                                   | Output                                   [7]
[8]   [1] "ab77d9f8eb5a3fae48c7e63b2ee52d5a" -   [1] "932c98c86dced4ef17094f8a70b35a04" [8]

* Run `testthat::snapshot_accept('as_matrix')` to accept the change.
* Run `testthat::snapshot_review('as_matrix')` to interactively review the change.

I don't understand what has changed. Any ideas?

fweber144 commented 2 months ago

I'm getting these errors when running tests

Failure (�]8;line = 225:col = 7;file:///u/77/ave/unix/proj/projpred/tests/testthat/test_as_matrix.R�test_as_matrix.R:225:7�]8;;�): as.matrix.projection() works
Snapshot of code has changed:
    old                                      | new                                         
[5] Code                                     | Code                                     [5]
[6]   print(rlang::hash(m))                  |   print(rlang::hash(m))                  [6]
[7] Output                                   | Output                                   [7]
[8]   [1] "ab77d9f8eb5a3fae48c7e63b2ee52d5a" -   [1] "932c98c86dced4ef17094f8a70b35a04" [8]

* Run `testthat::snapshot_accept('as_matrix')` to accept the change.
* Run `testthat::snapshot_review('as_matrix')` to interactively review the change.

I don't understand what has changed. Any ideas?

Have you updated to R 4.4.0 and/or relevant packages like the Matrix package? I did so myself and this gave me updated snapshots, too.

fweber144 commented 2 months ago

In commit b19dfcd7db6f77b8bf53f63902492e70903662b2, you switched to roxygen2 version 7.3.1, which now gives me

✖ formula.R:808: S3 method `formula.gamm4` needs @export or @exportS3method tag.
✖ divergence_minimizers.R:1216: S3 method `predict.gamm4` needs @export or @exportS3method tag.
✖ divergence_minimizers.R:1179: S3 method `predict.subfit` needs @export or @exportS3method tag.

when running devtools::document(). I'll open a new issue for this.

avehtari commented 2 months ago
fweber144 commented 2 months ago
  • I have R 4.3.3, but it seems updating some other package updated Matrix to 1.6-5 (2024-01-06) (which happens to be the last release for pre 4.4, and 4.4 requires Matrix 1.7+)

Ok, then the snapshot changes are probably due to the Matrix package update. I'll check this on my machine asap.

* I didn't intentionally switch roxygen2 version

No problem, I am definitely in favor of using the most recent roxygen2 version. And I think it shouldn't be too much work to get rid of those warnings (see #501).

fweber144 commented 1 month ago
  • I have R 4.3.3, but it seems updating some other package updated Matrix to 1.6-5 (2024-01-06) (which happens to be the last release for pre 4.4, and 4.4 requires Matrix 1.7+)

Ok, then the snapshot changes are probably due to the Matrix package update. I'll check this on my machine asap.

As far as I can tell, any snapshot changes should be due to package updates.

fweber144 commented 1 month ago

I have added a NEWS entry in commit 87331d6c57028b488ed999d57141ea41aba461d1. Please check if it is ok for you. Afterwards (keeping in mind my final comments above: https://github.com/stan-dev/projpred/pull/498#discussion_r1631670167 and https://github.com/stan-dev/projpred/pull/498#discussion_r1631677716), I think this PR is ready to be merged (I'll run R CMD check once again, though).

avehtari commented 1 month ago

I fixed the two sentences you mentioned

fweber144 commented 1 month ago

R CMD check succeeded. Merging now.