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 multi-output support to GP Latent #7471

Merged
merged 3 commits into from
Aug 30, 2024
Merged

Add multi-output support to GP Latent #7471

merged 3 commits into from
Aug 30, 2024

Conversation

AlexAndorra
Copy link
Contributor

@AlexAndorra AlexAndorra commented Aug 22, 2024

Porting over #7226 as it got staled a few months ago.
Ported the changes from @hchen19 and also added dims support to gp.Latent.prior. Ready for review 🍾

Closes #7152


📚 Documentation preview 📚: https://pymc--7471.org.readthedocs.build/en/7471/

@bwengals
Copy link
Contributor

looks good! not sure whats up with the unrelated test failures. Would it be possible to add @hchen19 as an author in the commit? Looks like its pretty easy here.

@hchen19
Copy link
Contributor

hchen19 commented Aug 23, 2024

@bwengals and @AlexAndorra , thanks for the suggestions and reviews of this PR. I think the test fails might come from the changes of logprob and logcdf in the commit 48a8b6b

@AlexAndorra
Copy link
Contributor Author

Would it be possible to add @hchen19 as an author in the commit? Looks like its pretty easy here.

You're my hero @bwengals , that's what I was looking for 🥳
Will push this ASAP, and then you can approve the PR

@AlexAndorra
Copy link
Contributor Author

@bwengals I think that one is good to go 🍾
I added @hchen19 as co-author and fixed a typo in hsgp.prior where we weren't removing the first basis coeff when the user provided a combination of drop_first and centered parametrization

Copy link

codecov bot commented Aug 27, 2024

Codecov Report

Attention: Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.

Project coverage is 92.16%. Comparing base (799c98f) to head (d1ba4dd).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
pymc/gp/gp.py 94.11% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7471      +/-   ##
==========================================
- Coverage   92.17%   92.16%   -0.01%     
==========================================
  Files         103      103              
  Lines       17217    17220       +3     
==========================================
+ Hits        15869    15871       +2     
- Misses       1348     1349       +1     
Files with missing lines Coverage Δ
pymc/gp/hsgp_approx.py 88.39% <100.00%> (ø)
pymc/gp/gp.py 94.85% <94.11%> (-0.19%) ⬇️

... and 1 file with indirect coverage changes

@bwengals bwengals merged commit 90f20a2 into main Aug 30, 2024
22 checks passed
@Armavica
Copy link
Member

Armavica commented Aug 30, 2024

@AlexAndorra Just to let you know that it didn't work, you added a > character instead of a new line. It's visible on the github history where you are the only author:

image

For comparison, a commit with several authors:

image

Another way to proceed would have been to cherry-pick or rebase the original commits, which would have automatically added the dual attribution.

@bwengals
Copy link
Contributor

Again, thanks for your work on this @hchen19! My fault for not merging your branch when you were finished.

@bwengals
Copy link
Contributor

And of course I approved and merged it without refreshing the tab and seeing your comment @Armavica. Sorry guys that probably makes it a bit harder to fix. Is there any issue with ammending the commit?

@Armavica
Copy link
Member

I commented one hour after you merged, so you couldn't have seen it :) Amending the commit would require to exceptionally lift the force-push protection of the main branch, since it's still the last commit I would say it's not too bad, but I don't know if it already has been done and we should probably ask someone else than me

@hchen19
Copy link
Contributor

hchen19 commented Aug 30, 2024

Again, thanks for your work on this @hchen19! My fault for not merging your branch when you were finished.

Never mind @bwengals

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: Extend gp.Latent to allow for multiple outputs
4 participants