Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix pareto warnings #498

Merged
merged 24 commits into from
Jun 10, 2024
Merged

Fix pareto warnings #498

merged 24 commits into from
Jun 10, 2024

Conversation

avehtari
Copy link
Collaborator

Fixes #490

@avehtari avehtari requested a review from fweber144 April 12, 2024 18:40
@n-kall n-kall self-assigned this Apr 16, 2024
R/cv_varsel.R Outdated Show resolved Hide resolved
R/cv_varsel.R Outdated Show resolved Hide resolved
R/cv_varsel.R Outdated Show resolved Hide resolved
R/cv_varsel.R Outdated Show resolved Hide resolved
@fweber144
Copy link
Collaborator

fweber144 commented Apr 21, 2024

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
Copy link
Collaborator Author

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 ✖

R/cv_varsel.R Outdated Show resolved Hide resolved
R/cv_varsel.R Outdated Show resolved Hide resolved
determined by RStudio's coding and indentation style)
R/cv_varsel.R Outdated Show resolved Hide resolved
@fweber144
Copy link
Collaborator

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

@avehtari
Copy link
Collaborator Author

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?

@fweber144
Copy link
Collaborator

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.

R/cv_varsel.R Outdated Show resolved Hide resolved
@fweber144
Copy link
Collaborator

In commit b19dfcd, 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
Copy link
Collaborator Author

  • 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+)
  • I didn't intentionally switch roxygen2 version

@fweber144
Copy link
Collaborator

  • 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).

R/cv_varsel.R Outdated Show resolved Hide resolved
R/cv_varsel.R Outdated Show resolved Hide resolved
@fweber144
Copy link
Collaborator

  • 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
Copy link
Collaborator

I have added a NEWS entry in commit 87331d6. Please check if it is ok for you. Afterwards (keeping in mind my final comments above: #498 (comment) and #498 (comment)), I think this PR is ready to be merged (I'll run R CMD check once again, though).

@avehtari
Copy link
Collaborator Author

I fixed the two sentences you mentioned

@fweber144
Copy link
Collaborator

R CMD check succeeded. Merging now.

@fweber144 fweber144 merged commit 7cee559 into master Jun 10, 2024
@fweber144 fweber144 deleted the fix-pareto-warnings branch June 10, 2024 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't warn about slightly large khats
3 participants