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

HSGP misc fixes #7342

Merged
merged 7 commits into from
Jun 10, 2024
Merged

HSGP misc fixes #7342

merged 7 commits into from
Jun 10, 2024

Conversation

bwengals
Copy link
Contributor

@bwengals bwengals commented Jun 1, 2024

Description

  • Fix centering calculation (replace - with +) (nice catch @ulfaslak!)
  • Use x_range instead of x in approx_hsgp_hyperparams. I don't think the full x is needed anymore, could be missing something though.
  • Add more detail in docstring of approx_hsgp_hyperparams about usage.
  • Renamed Xs to X. The "s" stood for "scaled", for when the user had to center or mean subtract the incoming X. Now this is done automatically, so it can just be called X. Though minor, it is a breaking change. Would it be good to add a deprecation warning and keep the Xs syntax for a while?

Also, didn't make this change, but what do people think about not returning S from approx_hsgp_hyperparams, so it would just return m and c? The idea was that returning S here would be helpful for the user when centering things, but that's not really necessary anymore.


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

@bwengals bwengals requested a review from AlexAndorra June 1, 2024 07:01
Copy link

codecov bot commented Jun 1, 2024

Codecov Report

Attention: Patch coverage is 70.58824% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 92.37%. Comparing base (508a134) to head (7640a18).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7342      +/-   ##
==========================================
- Coverage   92.37%   92.37%   -0.01%     
==========================================
  Files         102      102              
  Lines       17208    17206       -2     
==========================================
- Hits        15896    15894       -2     
  Misses       1312     1312              
Files Coverage Δ
pymc/gp/hsgp_approx.py 88.39% <70.58%> (-0.13%) ⬇️

@AlexAndorra
Copy link
Contributor

Thannks @bwengals !

Renamed Xs to X. [...] Would it be good to add a deprecation warning and keep the Xs syntax for a while?

I think it's fine because it's gonna get used more now that we have examples and tutorials up, so better do the change now than later.

What do people think about not returning S from approx_hsgp_hyperparams, so it would just return m and c?

Agreed, we can get rid of that, as it makes the API simpler. Then we can probably get rid of the NamedTuple?
Note that all those changes need to be added to the tutorial part 1 that we just published.

Copy link
Contributor

@AlexAndorra AlexAndorra left a comment

Choose a reason for hiding this comment

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

Almost all good @bwengals !
Just one change I highlighted, plus getting rid of returning S in the approx function (as well as updating the NB on pymc-examples)

pymc/gp/hsgp_approx.py Outdated Show resolved Hide resolved
@bwengals bwengals requested a review from juanitorduz June 1, 2024 22:26
Comment on lines +399 to +400
self._X_center = (pt.max(X, axis=0) + pt.min(X, axis=0)).eval() / 2
Xs = X - self._X_center # center for accurate computation
Copy link
Contributor

@juanitorduz juanitorduz Jun 2, 2024

Choose a reason for hiding this comment

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

Small suggestion: Could a unit test be added to check this so that we are sure future changes won't break the signs? We could just wrap this little logic into a small axillary function so that we can easily test it.

@juanitorduz
Copy link
Contributor

This looks great! Thanks! I agree with Alex's comments. I just left a small suggestion above :)

Copy link
Contributor

@AlexAndorra AlexAndorra left a comment

Choose a reason for hiding this comment

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

Just have a last question @bwengals

pymc/gp/hsgp_approx.py Outdated Show resolved Hide resolved
@bwengals
Copy link
Contributor Author

bwengals commented Jun 6, 2024

Sounds good, I can make these changes on Friday

@AlexAndorra
Copy link
Contributor

I got some time, so I just pushed the change to the formula for S @bwengals 🍾
Feel free to merge if it looks good and tests pass

@bwengals bwengals requested a review from AlexAndorra June 10, 2024 18:17
@AlexAndorra AlexAndorra merged commit 4f76576 into pymc-devs:main Jun 10, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants