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

REF: group-timepoints/engraftment pipeline take to_baseline param #71

Merged
merged 24 commits into from
Jun 24, 2024

Conversation

cherman2
Copy link
Contributor

@cherman2 cherman2 commented Feb 14, 2024

This PR refactors group-timepoints action and engraftment pipeline allow "to_baseline" parameter.

This PR changed

  • group-timepoints now has distance_to and baseline_timepoint param.
  • engraftment pipeline now has distance_to and baseline_timepoint param.
  • error handling for mis-matching allowed parameters. users can pass distance_to = donor and a reference column or distance_to = baseline and baseline_timepoint parameter.
  • refactored group-timepoints to prepare a dist for to_baseline comparison.

TODO:

  • Tests

@cherman2 cherman2 marked this pull request as draft February 14, 2024 15:22
@cherman2 cherman2 marked this pull request as ready for review May 3, 2024 15:32
@cherman2 cherman2 added this to the Manuscript Prep milestone May 23, 2024
@cherman2 cherman2 requested a review from lizgehret May 30, 2024 17:34
Copy link
Member

@lizgehret lizgehret left a comment

Choose a reason for hiding this comment

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

this all generally lgtm @cherman2! all of the comments below are just tiny formatting adjustments (from single quotes to ``) - this will just format those params nicely in the command line.

q2_fmt/_engraftment.py Outdated Show resolved Hide resolved
q2_fmt/_engraftment.py Outdated Show resolved Hide resolved
q2_fmt/_engraftment.py Outdated Show resolved Hide resolved
q2_fmt/_engraftment.py Outdated Show resolved Hide resolved
q2_fmt/_engraftment.py Outdated Show resolved Hide resolved
q2_fmt/tests/test_engraftment.py Outdated Show resolved Hide resolved
q2_fmt/tests/test_engraftment.py Outdated Show resolved Hide resolved
q2_fmt/tests/test_engraftment.py Outdated Show resolved Hide resolved
q2_fmt/tests/test_engraftment.py Outdated Show resolved Hide resolved
q2_fmt/tests/test_engraftment.py Outdated Show resolved Hide resolved
@cherman2
Copy link
Contributor Author

Thanks @lizgehret. I definitely agree that `` is more readable. I'll remember to do that next time i'm adding error messages!

@cherman2
Copy link
Contributor Author

cherman2 commented Jun 12, 2024

These tests failures are happening when my typeMap for group-column = false. I need to figure out what the other type maps are doing to make this work. I am kinda getting confused between my PRs. I feel like this PR shouldn't have group-column (that should come from #78)

@lizgehret
Copy link
Member

These tests failures are happening when my typeMap for group-column = false. I need to figure out what the other type maps are doing to make this work. I am kinda getting confused between my PRs. I feel like this PR shouldn't have group-column (that should come from #78)

Oh weird, I don't see those test failures here, only in #78 (at least in these latest CI runs). I'll reply on that PR re: those particular test failures (the ones in this PR aren't interesting).

@cherman2
Copy link
Contributor Author

You are right they are only on #78. I need to do a better job of naming concurrence prs . I think I am literally confusing these prs because they both start with REF: group-timepoints.

@cherman2 cherman2 requested a review from lizgehret June 13, 2024 16:52
@lizgehret lizgehret changed the title REF: group-timepoints/engraftment pipline take to_baseline param REF: group-timepoints/engraftment pipeline take to_baseline param Jun 19, 2024
Copy link
Member

@lizgehret lizgehret left a comment

Choose a reason for hiding this comment

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

all very small things @cherman2 - once these are updated, i'll pull this down locally to make sure all tests pass and then this should be gtm!

q2_fmt/_engraftment.py Outdated Show resolved Hide resolved
q2_fmt/_engraftment.py Outdated Show resolved Hide resolved
q2_fmt/_engraftment.py Outdated Show resolved Hide resolved
q2_fmt/_engraftment.py Outdated Show resolved Hide resolved
q2_fmt/_engraftment.py Outdated Show resolved Hide resolved
q2_fmt/tests/test_engraftment.py Outdated Show resolved Hide resolved
@lizgehret lizgehret assigned cherman2 and unassigned lizgehret Jun 20, 2024
@cherman2 cherman2 requested a review from lizgehret June 20, 2024 18:11
@cherman2 cherman2 assigned lizgehret and unassigned cherman2 Jun 20, 2024
Copy link
Member

@lizgehret lizgehret left a comment

Choose a reason for hiding this comment

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

Okay @cherman2, this all lgtm - when I replace GroupDist instances (still present on this branch) with Dist1D and run tests locally, they all pass. Would you prefer I wait until #78 is merged to merge this one? Tests won't pass until that PR is merged b/c of the type updates - but realistically it'll be fine either way (since we're going to get that one merged soon too).

@lizgehret lizgehret assigned cherman2 and unassigned lizgehret Jun 24, 2024
@cherman2
Copy link
Contributor Author

I guess I think just merge it because my CI isn't set-up yet and we will be merging #78 soon. but I definitely defer to your opinion!

@lizgehret lizgehret merged commit b63a770 into qiime2:dev Jun 24, 2024
2 of 4 checks passed
@lizgehret lizgehret deleted the to-baseline branch June 24, 2024 20:23
@cherman2 cherman2 removed their assignment Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

2 participants