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

Improve readability and add comments. #138

Merged
merged 1 commit into from Nov 6, 2022

Conversation

daniel-dodd
Copy link
Member

Minor addition of comments and code readability improvements [e.g., replacing long attributes such as self.prior.kernel(...) with kernel = self.prior.kernel then kernel(...)]. Most major changes are to the progress bar.

  • Bugfix
  • Feature
  • [X ] Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

@codecov
Copy link

codecov bot commented Nov 6, 2022

Codecov Report

Merging #138 (9ee096a) into v0.5_update (ddfbfab) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@               Coverage Diff               @@
##           v0.5_update     #138      +/-   ##
===============================================
+ Coverage        98.27%   98.32%   +0.05%     
===============================================
  Files               15       15              
  Lines             1332     1372      +40     
===============================================
+ Hits              1309     1349      +40     
  Misses              23       23              
Flag Coverage Δ
unittests 98.32% <100.00%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
gpjax/kernels.py 95.08% <ø> (ø)
gpjax/abstractions.py 100.00% <100.00%> (ø)
gpjax/config.py 100.00% <100.00%> (ø)
gpjax/gps.py 100.00% <100.00%> (+0.64%) ⬆️
gpjax/mean_functions.py 100.00% <100.00%> (ø)
gpjax/variational_families.py 100.00% <100.00%> (ø)
gpjax/variational_inference.py 97.75% <100.00%> (+0.13%) ⬆️
gpjax/covariance_operator.py 93.39% <0.00%> (-0.95%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Comment on lines +209 to +210
mean_function = self.mean_function
kernel = self.kernel
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see the advantage of this? It's only going to make stepping back through the code for new users harder.

@@ -31,11 +31,12 @@ class AbstractMeanFunction:
name: Optional[str] = "Mean function"

@abc.abstractmethod
def __call__(self, x: Float[Array, "N D"]) -> Float[Array, "N Q"]:
def __call__(self, x: Float[Array, "N D"], params: Dict) -> Float[Array, "N Q"]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where we not putting params as the first argument?

@thomaspinder thomaspinder merged commit 8a2cb02 into v0.5_update Nov 6, 2022
@thomaspinder thomaspinder deleted the improve_readability_for_v0.5 branch November 6, 2022 21:16
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.

None yet

2 participants