LRE Inference Functions

[purva-thakre]

Description

Fixes #2372

To Dos

• Add details to docstrings
• Add unit tests with chunking
• Make mypy happy
• Unit tests for sample matrix
• Function and unit test for linear combination coefficients function

---

License

• I license this contribution under the terms of the GNU GPL, version 3 and grant Unitary Fund the right to provide additional permissions as described in section 7 of the GNU GPL, version 3.

Before opening the PR, please ensure you have completed the following where appropriate.

• I added unit tests for new code.
• I used type hints in function signatures.
• I used Google-style docstrings for functions.
• I updated the documentation where relevant.
• Added myself / the copyright holder to the AUTHORS file

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.71%. Comparing base (02ebc4b) to head (f0c0293).
Report is 19 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2447      +/-   ##
==========================================
+ Coverage   98.70%   98.71%   +0.01%     
==========================================
  Files          88       89       +1     
  Lines        4083     4133      +50     
==========================================
+ Hits         4030     4080      +50     
  Misses         53       53              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[vprusso]

Overall looks great. Nice work, @purva-thakre !

Added some comments, but many of them are quite minor.

[natestemen]

Very cool stuff here.

I left some comments related to things Vincent raised. I think we can make some big simplifications with how we do monomials, but otherwise looking good.

[natestemen]

Nice changes! I'm hopeful we can simplify things a bit more, but I might not have the full picture in my head to understand why certain things need to be that way.

[purva-thakre]

@natestemen This is ready for a review with your suggestions.

I removed 2 unit tests in 42fd2b1 because np.sum is not behaving properly. I am looking into this.

When I add the unit test to check if all the coeffs sum up to 1, the tests fail. But if I manually check this value in a jupyter notebook by using np.sum(coeffs), they sum up close to 1. 🤷🏾‍♀️

I figured this was a float addition issue. Rounding the values up to 2 decimal places before calculating the sum is also not working here.

Edit: I think this is a bug in pytest.

[natestemen]

Nice clean up! Getting closer!

I'm lacking some understanding as to how the functions added here fit together, but maybe that will come in the final PR? This is usually where I would rely on the PR description to add context for the reviewer.

[natestemen]

Suggested change

	assert (
	expected_matrix - sample_matrix(test_circ, test_degree, 1)
	).all() <= 1e-3
	assert np.allclose(expected_matrix, sample_matrix(test_circ, test_degree, 1), atol=1e-3)

I don't think the test, as currently written, is testing what is intended. Calling .all on a numpy array returns a boolean indicating whether all the values of the array are truthy. Currently things are passing because expected_matrix is exactly equal to the output of sample_matrix, meaning the array is all 0's. The .all call then returns False and False <= 1e-3 evaluates to True. The right output, but incorrect path to get there.

Other tests need to be reworked to fix this as well.

[purva-thakre]

Thanks for catching this!

Apparently, the two failures I noticed in #2447 (comment) were related to this.

Because the tests passed, I thought there was a float addition error. I will work on finding a way around the for block where I was copying the array.

[natestemen]

This should either be removed, or we'll need to refactor _full_monomial_basis_term_exponents to speed it up. If we need to speed it up, what are the largest values it should be performant at?

[purva-thakre]

we'll need to refactor _full_monomial_basis_term_exponents to speed it up

TO DO: I was thinking of leaving this for the immediate future.

[natestemen]

Sounds good. Let's finish the full implementation and then come back to it. Please add TODO to the comment so it's a bit more discoverable.

[purva-thakre]

@natestemen @vprusso Does this sound like a better alternative to the previous version where I was copying the sample matrix?

Both of you pointed out memory issues with using .copy() on a numpy array.

[vprusso]

This seems okay from my perspective!

[purva-thakre]

Discussed with @natestemen to revert back to using .copy() on a numpy array because np.concatenate might have the same memory issues as .copy().

The code is more readable with .copy(). Will add this as a todo as discussed in #2447 (comment)

[vprusso]

Do we want to check whether it's close to 0.0 and add an appropriate debug message in case it is not?

[purva-thakre]

Discussed this with @natestemen yesterday. We are coming back to think about edge cases like this once the general LRE implementation is complete.

I'll go ahead and create an issue to log all the todos I need to tackle in the immediate future.

[vprusso]

Similar question here as to whether we want to provide more details to the end-user as to what the issue would have been here if this assert fails.

[purva-thakre]

See #2447 (comment)

[vprusso]

Should we add error handling?

if degree < 1:
    raise ValueError("Degree must be a positive integer")
if fold_multiplier < 1:
    raise ValueError("Fold multiplier must be a positive integer")
if num_chunks is not None and num_chunks < 1:
    raise ValueError("Number of chunks must be a positive integer")

[vprusso]

This seems okay from my perspective!

[natestemen]

Approved, but a few small cleanup things to do before merging!

Well done with this Purva!

[natestemen]

I believe the ordering we are using here is some sort of anti-lexicographic ordering within each "grade". E.g. with grade = 1, in lexicographic ordering 10 comes after 01 since 0 comes before 1. Maybe it's "graded reverse lexicographic ordering"?\

[purva-thakre]

raded reverse lexicographic ordering

No, this would be a different order. See section I of https://pi.math.cornell.edu/~dmehrle/notes/old/alggeo/07MonomialOrdersandDivisionAlgorithm.pdf

In the graded lexicographic order, the highest total degree is the largest along with the requirements of the lexicographic order which considers the difference between the exponents.

[natestemen]

Okay, so I think all the confusion boils down to a potential confusion in convention.

A tuple of exponents $(a_1, a_2, \ldots, a_n)$ can be taken to represent either $x_1^{a_1}x_2^{a_2}\cdots x_n^{a_n}$ or $x_1^{a_n}x_2^{a_{n-1}}\cdots x_n^{a_1}$. The choice here is unimportant in it's own right, but important that we know which one we are working in and stay consistent.

If we are working in the second convention (which seems a bit strange to me¹) then I think everything is correct. In my head, I was previously working with the former convention which means the graded lexicographic ordering on monomials with two terms and maximum degree two ($1 < x_2 < x_1 < x_2^2 < x_1x_2 < x_1^2$) would read

$$(0, 0) < (0, 1) < (1, 0) < (0, 2) < (1, 1) < (2, 0)$$

whereas the latter convention reads

$$(0, 0) < (1, 0) < (0, 1) < (2, 0) < (1, 1) < (0, 2).$$

The latter convention indeed yields what is currently implemented.

So the question is does $(0, 1)$ represent $x_1$ or $x_2$?

Footnotes

1. It's convention, though, not gospel, so 🤷🏻. ↩

[natestemen]

We typically use tests for this type of thing. Is there a reason we should have this assert in the code here?

[purva-thakre]

I am making my way through Dan Bader's 'Python Tricks: The Book' to help me write better code. He recommends using assertions like this.