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

address compute_grid_info() bug for partially regular grids #960

Closed
wants to merge 4 commits into from

Conversation

simonpcouch
Copy link
Contributor

Closes #959.

simonpcouch added a commit to tidymodels/extratests that referenced this pull request Nov 4, 2024
Copy link
Contributor Author

@simonpcouch simonpcouch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR fixes the compute_grid_info() refactor for two broken cases:

  • Grids that are partially regular
  • grid_regular() for both preprocessing and model parameters with no submodels

Comment on lines +346 to 348
if (isTRUE(model_iters_needed)) {
res$.iter_model <- res$.iter_model - 1
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not super happy with this pattern—feels like I'm missing a much simpler off-by-one fix?

@simonpcouch simonpcouch requested review from topepo and removed request for topepo November 4, 2024 18:07
@simonpcouch
Copy link
Contributor Author

Unrequesting, as there's an additional snap change in extratests🫠

simonpcouch added a commit that referenced this pull request Nov 12, 2024
These tests will fail with current `main` but pass with #960. Max will file a PR today with a new draft of the helper, and that helper ought to pass these tests.
@simonpcouch
Copy link
Contributor Author

Closing in favor of an incoming PR from Max!

@simonpcouch simonpcouch deleted the grid-959 branch November 12, 2024 16:46
@topepo topepo mentioned this pull request Nov 13, 2024
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.

bug in refactored compute_grid_info()
1 participant