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.

Sign up for GitHub

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

Jump to bottom

98/ Subset expanded grid of valid values by output type #107

Merged
merged 40 commits into from
Aug 16, 2024
Merged

98/ Subset expanded grid of valid values by output type #107

merged 40 commits into from
Aug 16, 2024

Conversation

annakrystalli
Copy link
Member

This PR resolves #98 by introducing an output_type argument to expand_model_out_grid() which allows for producing output type specific subsets of the expanded grid of valid values.

It also adds a bunch of low level internal functions for extracting information from round level configs

This will be used downstream in #93 as well as form the basis for #97

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 13, 2024 •
edited
Loading

🚀 Deployed on https://66bf14a4832d57dd48f48a96--hubvalidations-pr-previews.netlify.app

@annakrystalli
Copy link
Member Author

Apologies to the reviewer...I think I've misunderstood what rebasing was supposed to do...the last 5 commits which are somewhat polluting the PR, were through a rebase to add linting fixes made in #99 so I didn't think they'd show up here given they are already in main

@annakrystalli annakrystalli requested a review from elray1 August 13, 2024 10:00
elray1
elray1 approved these changes Aug 13, 2024
View reviewed changes
Copy link
Contributor

@elray1 elray1 left a comment

Choose a reason for hiding this comment

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

One question of substance if we should just always throw an error if an invalid output type is specified. Then I had questions about indentation but it seems like linting passed so OK to just move on from those. I've approved as good enough to merge.

tests/testthat/_snaps/expand_model_out_grid.md Show resolved Hide resolved
tests/testthat/test-check_tbl_spl_compound_tid.R Outdated Show resolved Hide resolved
tests/testthat/test-check_tbl_spl_compound_tid.R Outdated Show resolved Hide resolved
tests/testthat/test-check_tbl_spl_n.R Outdated Show resolved Hide resolved
tests/testthat/test-check_tbl_spl_n.R Outdated Show resolved Hide resolved
tests/testthat/test-check_tbl_spl_non_compound_tid.R Outdated Show resolved Hide resolved
@annakrystalli
Copy link
Member Author

Thanks @elray1 ! Re: the linting, it's what styler output and as mentioned the linting passed so I think we should leave it.

One question of substance if we should just always throw an error if an invalid output type is specified.

I think that's reasonable.

@zkamvar zkamvar mentioned this pull request Aug 14, 2024
Open
@annakrystalli
Make ignored derived task id message clearer
70afb1d
@annakrystalli
rerun snapshot
74ad9a4
@annakrystalli
remove superfluous check
1811ede
@annakrystalli
Merge pull request #108 from hubverse-org/93/add-derived-tid-arg
2cce180 
93/add derived_task_id argument
@annakrystalli
merge earlier PRs
4a45645 
Merge branch '98/subset-grid-by-out-type' into 93/3-batch-value-validation

# Conflicts:
#	R/check_tbl_spl_compound_tid.R
#	R/v3-sample-utils.R
#	tests/testthat/test-check_tbl_spl_compound_tid.R
#	tests/testthat/test-check_tbl_spl_non_compound_tid.R
@annakrystalli
Throw error if invalid output_type supplied instead of ignoring.
edbf939
@annakrystalli
add output_types and derived_task_ids arguments to submission_tmpl
28715b3
@annakrystalli
Add note on derived task IDs in pkgdown docs
8ec8261
@annakrystalli annakrystalli mentioned this pull request Aug 15, 2024
Merged
@annakrystalli annakrystalli self-assigned this Aug 15, 2024
@annakrystalli annakrystalli merged commit ca8e22b into main Aug 16, 2024
8 checks passed
@annakrystalli annakrystalli deleted the 98/subset-grid-by-out-type branch August 16, 2024 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elray1 elray1 elray1 approved these changes

Assignees

@annakrystalli annakrystalli

Labels
performance
Projects
hubverse Development overview
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow subsetting of expand_model_out_val_grid() by output type
2 participants
@annakrystalli @elray1