Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
LRE Inference Functions #2447
LRE Inference Functions #2447
Changes from all commits
428128e
90f22dc
634ea80
0e297a5
8b2ad8c
00b92c5
d61b1cc
7d8868b
c92a38a
c247619
6290f93
aef46ab
d382767
30c6914
f320b2b
378aad8
f539153
f1d1635
3ff1578
eb0f9d7
fd6be18
42fd2b1
83880e3
7e55d5f
80fe32c
48bae71
d81bd6f
d97c2f3
f0c0293
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 after01
since0
comes before1
. Maybe it's "graded reverse lexicographic ordering"?\There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 me1) 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
whereas the latter convention reads
The latter convention indeed yields what is currently implemented.
So the question is does$(0, 1)$ represent $x_1$ or $x_2$ ?
Footnotes
It's convention, though, not gospel, so 🤷🏻. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We typically use tests for this type of thing. Is there a reason we should have this
assert
in the code here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add error handling?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to check whether it's close to 0.0 and add an appropriate debug message in case it is not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.