pymc icon indicating copy to clipboard operation
pymc copied to clipboard

Add multi-output support to GP Latent

Open AlexAndorra opened this issue 1 year ago β€’ 5 comments

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/

AlexAndorra avatar Aug 22 '24 09:08 AlexAndorra

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.

bwengals avatar Aug 23 '24 16:08 bwengals

@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 48a8b6b4e110ab8b9cef904605196e556dbd2157

hchen19 avatar Aug 23 '24 20:08 hchen19

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 avatar Aug 24 '24 18:08 AlexAndorra

@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

AlexAndorra avatar Aug 27 '24 09:08 AlexAndorra

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 107 commits behind head on main.

Files with missing lines Patch % Lines
pymc/gp/gp.py 94.11% 1 Missing :warning:
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%) :arrow_down:

... and 1 file with indirect coverage changes

codecov[bot] avatar Aug 27 '24 09:08 codecov[bot]

@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.

Armavica avatar Aug 30 '24 20:08 Armavica

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

bwengals avatar Aug 30 '24 22:08 bwengals

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?

bwengals avatar Aug 30 '24 22:08 bwengals

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

Armavica avatar Aug 30 '24 22:08 Armavica

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

Never mind @bwengals

hchen19 avatar Aug 30 '24 23:08 hchen19