Skip to content

Conversation

@rlouf
Copy link
Member

@rlouf rlouf commented Sep 30, 2021

Closes #36.

@rlouf rlouf force-pushed the welford-algorithm branch from f57c98f to f75f3a0 Compare October 4, 2021 16:10
@codecov
Copy link

codecov bot commented Oct 4, 2021

Codecov Report

Merging #38 (a268c79) into main (9011be2) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main       #38   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines          362       385   +23     
  Branches        14        17    +3     
=========================================
+ Hits           362       385   +23     
Impacted Files Coverage Δ
aehmc/algorithms.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9011be2...a268c79. Read the comment docs.

@rlouf rlouf force-pushed the welford-algorithm branch 2 times, most recently from c832a03 to e951110 Compare October 5, 2021 14:44
@rlouf
Copy link
Member Author

rlouf commented Oct 5, 2021

This is ready for review. The code will eventually go in aehmc/algorithms.py when #34 is merged.

@rlouf rlouf marked this pull request as ready for review October 5, 2021 14:46
Copy link
Member

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

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

Looks great! Just a minor question/change regarding one of the function arguments.

aehmc/welford.py Outdated

"""

def init(n_dims: TensorVariable):
Copy link
Member

Choose a reason for hiding this comment

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

Should this really be called n_dims and should it be typed as TensorVariable, instead of—say—int? According to its use, it looks like it should be called "size" or "length".

In general, it isn't possible to have a symbolic/variable number of dimensions in most—if not all—tensor libraries, including Aesara.

Copy link
Member Author

Choose a reason for hiding this comment

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

Replaced TensorVariable with int. The variable corresponds to the number of dimensions of the problem.

@rlouf rlouf force-pushed the welford-algorithm branch 2 times, most recently from 0ac9c77 to a268c79 Compare October 14, 2021 11:27
@rlouf
Copy link
Member Author

rlouf commented Oct 14, 2021

I made the requested change, and rebased on main after having merged #35

@rlouf rlouf merged commit 8a634df into main Oct 14, 2021
@rlouf rlouf deleted the welford-algorithm branch October 14, 2021 15:44
@brandonwillard brandonwillard added the enhancement New feature or request label Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Welford algorithm to compute covariance

3 participants