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

REF: group-timepoints to create Dist1D[nestedOrdered] #78

Merged
merged 19 commits into from
Jul 9, 2024
Merged

REF: group-timepoints to create Dist1D[nestedOrdered] #78

merged 19 commits into from
Jul 9, 2024

Conversation

cherman2
Copy link
Contributor

@cherman2 cherman2 commented May 14, 2024 •
edited
Loading

No description provided.

@cherman2 cherman2 marked this pull request as draft May 14, 2024 23:51
@cherman2
Copy link
Contributor Author

Linked to qiime2/q2-stats#21

@cherman2
Copy link
Contributor Author

Validator for Dist1D[Nested] here: qiime2/q2-stats#22

@cherman2 cherman2 changed the title NEW: ADD Prepare-group-timepoints REF: group-timepoints to create Dist1D[nestedOrdered] May 16, 2024
@cherman2 cherman2 marked this pull request as ready for review May 20, 2024 16:55
@cherman2 cherman2 added this to the Manuscript Prep milestone May 23, 2024
@cherman2 cherman2 requested a review from lizgehret May 29, 2024 18:10
@cherman2
Copy link
Contributor Author

Thanks @lizgehret for taking a look at this. Let me know if you need any clarification!

@cherman2 cherman2 mentioned this pull request Jun 4, 2024
Merged
lizgehret
lizgehret reviewed Jun 11, 2024
View reviewed changes
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.

overall this lgtm @cherman2, just a couple of stray print statements that could be removed!

q2_fmt/_engraftment.py Outdated Show resolved Hide resolved
q2_fmt/_engraftment.py Outdated Show resolved Hide resolved
cherman2
cherman2 commented Jun 11, 2024
View reviewed changes
q2_fmt/_engraftment.py Outdated Show resolved Hide resolved
cherman2
cherman2 commented Jun 11, 2024
View reviewed changes
q2_fmt/_engraftment.py Outdated Show resolved Hide resolved
cherman2
cherman2 commented Jun 11, 2024
View reviewed changes
q2_fmt/_engraftment.py Outdated Show resolved Hide resolved
@cherman2
Copy link
Contributor Author

Thanks @lizgehret,
Those suggestions have been addressed! I also edited a comment that mentioned GroupDists (commit dd4d19d)

@cherman2 cherman2 requested a review from lizgehret June 11, 2024 21:38
@lizgehret
Copy link
Member

Whoah, not sure what happened to CI here - setup & lint shouldn't be running twice. @cherman2 can you try pushing up another commit (maybe just fixing the lint error) to see if this was just a stochastic issue?

@cherman2
Copy link
Contributor Author

Seems like that fixed it @lizgehret

@cherman2 cherman2 mentioned this pull request Jun 12, 2024
Merged
1 task
@lizgehret
Copy link
Member

Hmm, this is interesting - your TypeMap looks identical to your T_subject, T_dependence one (which seems like it mirrors the use-case for this one). Were you getting these failures locally as well?

@cherman2
Copy link
Contributor Author

cherman2 commented Jun 12, 2024 •
edited
Loading

I don't remember getting these failures when I marked the PR ready for review but I am getting them locally. Seems like the subject map is doing a check that I am not doing. I think the check is on line 230 of _engraftment.py

edit: yup that was it

@cherman2
Copy link
Contributor Author

New error and explanation... I apparently didn't hook up group to engraftment pipeline, so when we pass group into grouptimes in the pipeline it errors. I will fix this but It might take me a minute since this isn't as minor of an edit and im OOO.

@lizgehret
Copy link
Member

New error and explanation... I apparently didn't hook up group to engraftment pipeline, so when we pass group into grouptimes in the pipeline it errors. I will fix this but It might take me a minute since this isn't as minor of an edit and im OOO.

Gotcha, that makes sense - just ping me when you're ready for another round of review!

@cherman2
Copy link
Contributor Author

Alright, I remembered why engraftment didn't get hooked up. I don't have the stats I need to use the pipeline effectively yet. So for now I think Dist1D[nestedOrdered] should only be possible outside of the engraftment pipeline. Once I have those stats I will hook up engraftment pipeline to make Dist1D[nestedOrdered]. Issue #80

@lizgehret
Copy link
Member

Roger that @cherman2, are you planning to do that in a separate PR? Or add to this one?

@cherman2
Copy link
Contributor Author

cherman2 commented Jul 2, 2024

Hi @lizgehret,
I have implemented "Dist1D[nestedOrdered] should only be possible outside of the engraftment pipeline." In this PR for now. I am currently sorting through merge conflicts between this and the to_basline pr that was merged last week. I will re-assign to you once its ready!

@lizgehret
Copy link
Member

Hi @lizgehret, I have implemented "Dist1D[nestedOrdered] should only be possible outside of the engraftment pipeline." In this PR for now. I am currently sorting through merge conflicts between this and the to_basline pr that was merged last week. I will re-assign to you once its ready!

SLAP!

@cherman2 cherman2 assigned lizgehret and unassigned cherman2 Jul 8, 2024
@cherman2
Copy link
Contributor Author

cherman2 commented Jul 8, 2024

@lizgehret, This PR is ready for you!

lizgehret
lizgehret reviewed Jul 9, 2024
View reviewed changes
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 - couple of super minor comments and then this will be ready to merge!

q2_fmt/_engraftment.py Outdated Show resolved Hide resolved
q2_fmt/_engraftment.py Outdated Show resolved Hide resolved
@lizgehret lizgehret assigned cherman2 and unassigned lizgehret Jul 9, 2024
cherman2
cherman2 commented Jul 9, 2024
View reviewed changes
q2_fmt/_engraftment.py Outdated Show resolved Hide resolved
@cherman2 cherman2 assigned lizgehret and unassigned cherman2 Jul 9, 2024
@cherman2
Copy link
Contributor Author

cherman2 commented Jul 9, 2024

Thanks @lizgehret
Tag you are it!

lizgehret
lizgehret approved these changes Jul 9, 2024
View reviewed changes
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.

LGTM, LGTM! 🚀

@lizgehret lizgehret merged commit d665398 into qiime2:dev Jul 9, 2024
4 checks passed
@lizgehret lizgehret deleted the prepare-timepoint-groups branch July 9, 2024 18:25
@lizgehret lizgehret assigned cherman2 and unassigned lizgehret Jul 9, 2024
@cherman2 cherman2 removed their assignment Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lizgehret lizgehret lizgehret approved these changes

Assignees
No one assigned
Labels
None yet
Projects
No open projects
2024.10 🎃
Status: Completed
Milestone
Manuscript Prep
Development

Successfully merging this pull request may close these issues.
2 participants
@cherman2 @lizgehret