new unified get_ref_info

[munoztd0]

Pull Request

Fixes #295

Context

junco has at 3 ways to resolve a reference group from ref_path + .spl_context:

1. get_ref_info() exported, returns ref_group + in_ref_col, handles NULL cases. Used by most a_* functions.
2. h_get_trtvar_refpath() exported (should it even be export ?), returns trt_var / ctrl_grp only, not ref_group. Uses tail(...,1) assumption on the split context.
3. Inline in a_summarize_aval_chg_diff_j reimplements both above from scratch, with a different ref_group construction strategy, (direct filter vs .spl_context index).

Fix

Unified all into get_ref_info() + created documentation vignette

Questions

Contributing get_ref_info to tern after we align all ?

Checks

• (Have you updated the NEWS.md ?)
• (Have you added proper tests for new functions/features ?)
• (Have you added new functions to the pkgdown.yml ?)
• (Have you run document() on new functions ?)

[wwojciech]

Hi @munoztd0 - I slightly streamlined the code.

[wwojciech]

@munoztd0 - Please also check all of this with @iaugusty

[wwojciech]

Changes required.

[iaugusty]

as scda.test https://github.com/munoztd0/scda.test/actions/runs/32381205219 confirms consistent behavior I approve the PR

[wwojciech]

Hi @munoztd0

Thank you very much for all the updates you made. I carefully reviewed and reconsidered the updated h_get_trtvar_refpath() implementation. Apart from the one or two issues I commented on, I think the overall design and the intended use case of the function should be improved. At the moment, it mostly performs checks on the data, while for the majority of its other operations, it simply extracts elements from ref_path. I do not think this is an optimal design for the function.

What I changed:

I redesigned (and renamed h_get_trtvar_refpath() -> h_get_cur_trt_grp()) it so that its purpose is more consistent and streamlined. Anything that is not computed by this function is now taken directly from ref_path by the functions that were using h_get_trtvar_refpath().

I fully updated a_freq_j() to use h_get_cur_trt_grp(), so you can see how this is intended to be used.

Important changes to be recorded here:

1. trt_var is no longer identified by its position on the column-split path given by the user. I know this used to be the case, but I now think this is unnecessarily complicated. However, I did not implement this requirement in the past (probably @iaugusty did), so please check with her whether relaxing this assumption is appropriate.

2. This condition from the old h_get_trtvar_refpath():

   ref_trt_grp %in% levels(df[[cur_trt_var]])

   is no longer enforced. I don't see any reason why this should be enforced by h_get_trtvar_refpath() itself. Instead, this should be asserted by the other functions that eventually use its results.

   However, just to be on the safe side in case these "other" functions are not properly coded, I added an explicit check in a_freq_j():

   stopifnot(ctrl_grp %in% levels(df[[trt_var]]))

To summarise: h_get_cur_trt_grp() is now completely finished and documented. You don't need to change it unless you spot something that should be changed or improved.

What remains:

1. Add unit tests for strict_match().

2. Add unit tests for h_get_cur_trt_grp() and remove the tests for h_get_trtvar_refpath().

3. Update all the code (afuns) that uses the old h_get_trtvar_refpath(). I did this completely for a_freq_j(), so you can use it as a reference.

4. Check with @iaugusty whether relaxing the assumption about the fixed position versus matching the treatment variable would be appropriate. If she feels that this assumption is important for some part of the upstream code, please simply update h_get_cur_trt_grp() so that it accepts the new argument, trt_var_pos, representing the treatment variable position on the current column-split path. The order of arguments should then be: trt_var, trt_var_pos, .spl_context.

   If she feels that the assumption should be maintained, could you please ask her to provide a specific justification in the code indicating where this assumption is required? I’m asking mainly to make sure we fully understand the dependency here, as I have a suspicion that the assumption may not actually be necessary. If that is the case, there may be another design assumption elsewhere in the code that is causing this dependency, and it would be useful to identify it.

5. Check with @iaugusty whether we can skip:

   stopifnot(ctrl_grp %in% levels(df[[trt_var]]))

   at the place where it is now (see a_freq_j()) or whether it should eventually be moved to the function that actually uses ctrl_grp, for example get_ctrl_subset(), which is the very next function that uses ctrl_grp variable after it is created (see a_freq_j()).

   I think this check should be removed and any such checks should be performed in the appropriate downstream functions. Note that this check actually depends only on ref_path and df.

6. Notice the assertions for ref_path that I added at the beginning of a_freq_j(). Similar (or the same) assertions should be added to all other functions where ref_path is used. However, in the context of this PR, please limit yourself to the functions that use h_get_cur_trt_grp().

   Perhaps it would be useful to introduce a small assertion helper, such as assert_col_path(x, ..., min.len, len), since we often need to perform two separate checks when validating the column-split path.

Feel free to reach out if any further clarification is needed.

[munoztd0]

Hi @munoztd0

Thank you very much for all the updates you made. I carefully reviewed and reconsidered the updated h_get_trtvar_refpath() implementation. Apart from the one or two issues I commented on, I think the overall design and the intended use case of the function should be improved. At the moment, it mostly performs checks on the data, while for the majority of its other operations, it simply extracts elements from ref_path. I do not think this is an optimal design for the function.

What I changed:

I redesigned (and renamed h_get_trtvar_refpath() -> h_get_cur_trt_grp()) it so that its purpose is more consistent and streamlined. Anything that is not computed by this function is now taken directly from ref_path by the functions that were using h_get_trtvar_refpath().

I fully updated a_freq_j() to use h_get_cur_trt_grp(), so you can see how this is intended to be used.

Important changes to be recorded here:

1. trt_var is no longer identified by its position on the column-split path given by the user. I know this used to be the case, but I now think this is unnecessarily complicated. However, I did not implement this requirement in the past (probably @iaugusty did), so please check with her whether relaxing this assumption is appropriate.

2. This condition from the old h_get_trtvar_refpath():

   ref_trt_grp %in% levels(df[[cur_trt_var]])

   is no longer enforced. I don't see any reason why this should be enforced by h_get_trtvar_refpath() itself. Instead, this should be asserted by the other functions that eventually use its results.
   However, just to be on the safe side in case these "other" functions are not properly coded, I added an explicit check in a_freq_j():

   stopifnot(ctrl_grp %in% levels(df[[trt_var]]))

To summarise: h_get_cur_trt_grp() is now completely finished and documented. You don't need to change it unless you spot something that should be changed or improved.

What remains:

1. Add unit tests for strict_match().

2. Add unit tests for h_get_cur_trt_grp() and remove the tests for h_get_trtvar_refpath().

3. Update all the code (afuns) that uses the old h_get_trtvar_refpath(). I did this completely for a_freq_j(), so you can use it as a reference.

4. Check with @iaugusty whether relaxing the assumption about the fixed position versus matching the treatment variable would be appropriate. If she feels that this assumption is important for some part of the upstream code, please simply update h_get_cur_trt_grp() so that it accepts the new argument, trt_var_pos, representing the treatment variable position on the current column-split path. The order of arguments should then be: trt_var, trt_var_pos, .spl_context.
   If she feels that the assumption should be maintained, could you please ask her to provide a specific justification in the code indicating where this assumption is required? I’m asking mainly to make sure we fully understand the dependency here, as I have a suspicion that the assumption may not actually be necessary. If that is the case, there may be another design assumption elsewhere in the code that is causing this dependency, and it would be useful to identify it.

5. Check with @iaugusty whether we can skip:

   stopifnot(ctrl_grp %in% levels(df[[trt_var]]))

   at the place where it is now (see a_freq_j()) or whether it should eventually be moved to the function that actually uses ctrl_grp, for example get_ctrl_subset(), which is the very next function that uses ctrl_grp variable after it is created (see a_freq_j()).
   I think this check should be removed and any such checks should be performed in the appropriate downstream functions. Note that this check actually depends only on ref_path and df.

6. Notice the assertions for ref_path that I added at the beginning of a_freq_j(). Similar (or the same) assertions should be added to all other functions where ref_path is used. However, in the context of this PR, please limit yourself to the functions that use h_get_cur_trt_grp().
   Perhaps it would be useful to introduce a small assertion helper, such as assert_col_path(x, ..., min.len, len), since we often need to perform two separate checks when validating the column-split path.

Feel free to reach out if any further clarification is needed.

Could you confirm that those remarks would be the last ones on the refactoring of any of those functions ?

[wwojciech]

Hi @munoztd0
Thank you very much for all the updates you made. I carefully reviewed and reconsidered the updated h_get_trtvar_refpath() implementation. Apart from the one or two issues I commented on, I think the overall design and the intended use case of the function should be improved. At the moment, it mostly performs checks on the data, while for the majority of its other operations, it simply extracts elements from ref_path. I do not think this is an optimal design for the function.

What I changed:

I redesigned (and renamed h_get_trtvar_refpath() -> h_get_cur_trt_grp()) it so that its purpose is more consistent and streamlined. Anything that is not computed by this function is now taken directly from ref_path by the functions that were using h_get_trtvar_refpath().
I fully updated a_freq_j() to use h_get_cur_trt_grp(), so you can see how this is intended to be used.
Important changes to be recorded here:

1. trt_var is no longer identified by its position on the column-split path given by the user. I know this used to be the case, but I now think this is unnecessarily complicated. However, I did not implement this requirement in the past (probably @iaugusty did), so please check with her whether relaxing this assumption is appropriate.

2. This condition from the old h_get_trtvar_refpath():

   ref_trt_grp %in% levels(df[[cur_trt_var]])

   is no longer enforced. I don't see any reason why this should be enforced by h_get_trtvar_refpath() itself. Instead, this should be asserted by the other functions that eventually use its results.
   However, just to be on the safe side in case these "other" functions are not properly coded, I added an explicit check in a_freq_j():

   stopifnot(ctrl_grp %in% levels(df[[trt_var]]))

To summarise: h_get_cur_trt_grp() is now completely finished and documented. You don't need to change it unless you spot something that should be changed or improved.

What remains:

1. Add unit tests for strict_match().

2. Add unit tests for h_get_cur_trt_grp() and remove the tests for h_get_trtvar_refpath().

3. Update all the code (afuns) that uses the old h_get_trtvar_refpath(). I did this completely for a_freq_j(), so you can use it as a reference.

4. Check with @iaugusty whether relaxing the assumption about the fixed position versus matching the treatment variable would be appropriate. If she feels that this assumption is important for some part of the upstream code, please simply update h_get_cur_trt_grp() so that it accepts the new argument, trt_var_pos, representing the treatment variable position on the current column-split path. The order of arguments should then be: trt_var, trt_var_pos, .spl_context.
   If she feels that the assumption should be maintained, could you please ask her to provide a specific justification in the code indicating where this assumption is required? I’m asking mainly to make sure we fully understand the dependency here, as I have a suspicion that the assumption may not actually be necessary. If that is the case, there may be another design assumption elsewhere in the code that is causing this dependency, and it would be useful to identify it.

5. Check with @iaugusty whether we can skip:

   stopifnot(ctrl_grp %in% levels(df[[trt_var]]))

   at the place where it is now (see a_freq_j()) or whether it should eventually be moved to the function that actually uses ctrl_grp, for example get_ctrl_subset(), which is the very next function that uses ctrl_grp variable after it is created (see a_freq_j()).
   I think this check should be removed and any such checks should be performed in the appropriate downstream functions. Note that this check actually depends only on ref_path and df.

6. Notice the assertions for ref_path that I added at the beginning of a_freq_j(). Similar (or the same) assertions should be added to all other functions where ref_path is used. However, in the context of this PR, please limit yourself to the functions that use h_get_cur_trt_grp().
   Perhaps it would be useful to introduce a small assertion helper, such as assert_col_path(x, ..., min.len, len), since we often need to perform two separate checks when validating the column-split path.

Feel free to reach out if any further clarification is needed.

Could you confirm that those remarks would be the last ones on the refactoring of any of those functions ?

Based on the current state of our knowledge and my understanding of these functions, I have no further remarks and do not currently see a need for additional updates.

That said, I would also note that the overall use case and intended purpose of these functions were not sufficiently clear. There was a lack of proper documentation and examples, and the design and API of the functions were in several places confusing. There were also some assumptions in the existing implementation that were not obvious from the code or otherwise documented.

I had expected these aspects to be clarified, but since this was not the case, I had to determine the intended behavior myself by going through and debugging the existing implementation. This is why the review required several iterations of corrections and refactoring.

Of course, if new information or issues arise, including from the still-open questions to @iaugusty , this may need to be revisited.

[wwojciech]

One critical change required as I explained in my comment. Otherwise - approved.