Generalized monomial orders for Laurent polynomials #37196
Conversation
…ccept new generalized orders
| """ | ||
| self._n = n | ||
| self._n_cones = self._n + 1 | ||
| self._cones = build_cones(self._n) |
There was a problem hiding this comment.
It is really useful to have this helper function build_cones?
Same remark for get_group_order and get_score_function.
| def greatest_tuple(self,a,b=None): | ||
| r""" | ||
| Return the greatest of the two tuples ``a`` and ``b`` if ``b`` is not None | ||
| Return the greatest tuple in the list ``L`` if ``b`` is not a tuple |
There was a problem hiding this comment.
| Return the greatest tuple in the list ``L`` if ``b`` is not a tuple | |
| Return the greatest tuple in the list ``a`` if ``b`` is not a tuple |
| r""" | ||
| Create a generalized monomial order. | ||
|
|
||
| INPUTS: |
There was a problem hiding this comment.
I think we should write INPUTS without S (even if there are multiple inputs).
Moreover, the list of inputs should not be indented.
|
Documentation preview for this PR (built with commit e3a87e8; changes) is ready! 🎉 |
| - Group order: this can be any class with a 'greatest_tuple' method returning | ||
| the greatest of two tuples based on a group order on `\ZZ^n`. | ||
| Available choices include 'lex'. |
There was a problem hiding this comment.
It's not really the case.
However, I think that accepting any such class sounds like a good idea.
| if group_order in ["lex"]: | ||
| self._group_order = TermOrder(name=group_order) | ||
| else: | ||
| raise ValueError("Available group order are: 'lex'") |
There was a problem hiding this comment.
As I said above, I would add a test like elif isinstance(group_order, TermOrder): here.
| elif score_function == "degmin": | ||
| self._score_function = degmin_score_function | ||
| else: | ||
| raise ValueError("Available score function are: 'min', 'degmin'") |
There was a problem hiding this comment.
Again, why not giving the option to the user to pass in any score function ?
| self._score_function_name = score_function | ||
|
|
||
| def _repr_(self): | ||
| r""" |
There was a problem hiding this comment.
I thnk you should add a description here, something like Return a string representation of this order.
Add you should also add a black line after TESTS::.
| return L[0] | ||
| else: | ||
| a = L[0] | ||
| b = self.greatest_tuple(*L[1:]) |
There was a problem hiding this comment.
I don't really like this recursive call because it implies all useless checkings.
Why not programming this imperatively with a for loop?
| def min_score_function(t): | ||
| return -min(0,*t) | ||
|
|
||
| def degmin_score_function(t): | ||
| return sum(t) -(len(t)+1)*min(0,*t) |
There was a problem hiding this comment.
These two functions should be documented.
You also have a problem with identation.
Adding an implementation of generalized order defined by a conic decomposition and a compatible total order on$\mathbb{Z}^n$ . This will be useful to implement the computation of Gröbner basis of ideals over Laurent polynomials.
📝 Checklist