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

add failing test for check of ascending columns #188

Merged
merged 3 commits into from
Jan 7, 2025

Conversation

elray1
Copy link
Contributor

@elray1 elray1 commented Jan 7, 2025

This PR adds a currently-failing test case for the check of ascending values. The test hub has two task groups with different quantile levels requested: The first asks for quantiles at levels 0.1, 0.5, and 0.9, while the second asks for quantiles at 0.1, 0.2, ..., 0.9. A test submission file with correct ordering fails because its quantiles are re-ordered to the levels [0.1, 0.5, 0.9, 0.2, 0.3, 0.4, 0.6, 0.7, 0.8]

@elray1
Copy link
Contributor Author

elray1 commented Jan 7, 2025

I thought that a solution might be to update order_output_type_ids as follows, essentially doing the join/reordering within each task group:

order_output_type_ids <- function(tbl, config, types = c("cdf", "quantile")) {
  # step 1: create a lookup table from the config
  cdf_and_quantile <- config$output_type %in% types
  order_ref <- config[cdf_and_quantile, , drop = FALSE]
  # step 2: join
  tbl$output_type_id <- as.character(tbl$output_type_id)
  tbl_cols <- colnames(tbl)
  join_cols <- tbl_cols[tbl_cols != "value"]
  dplyr::inner_join(order_ref, tbl, by = join_cols)
}

However, this led to errors like:

Error in `dplyr::inner_join(order_ref, tbl, by = join_cols)`: Can't join `x$origin_date` with `y$origin_date` due to incompatible types.
i `x$origin_date` is a <character>.
i `y$origin_date` is a <date>.

So I guess we would need to update the tbl data types to the expected hub data types to enable that join... which should be OK as long as check_tbl_col_types ran previously and passed?

elray1 and others added 2 commits January 7, 2025 11:21
This aligns with how the data would be presented to the function during
validation
@zkamvar
Copy link
Member

zkamvar commented Jan 7, 2025

made changes in c8b9e67 after discussion with @elray1

@zkamvar zkamvar merged commit b76cf0f into znk/78/fix-ascending-check Jan 7, 2025
@zkamvar zkamvar deleted the elr/ascending_check_test_case branch January 7, 2025 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants