Conversation
|
Documentation preview for this PR (built with commit daa4045; changes) is ready! 🎉 |
vincentmacri
left a comment
vincentmacri
left a comment
There was a problem hiding this comment.
Possibly related PR when I fixed something similar: #37972
The implementation of divisors could benefit from a major rewrite, but this fix looks good enough to address the bug report. Just one suggestion.
| # once Sage can deal with divisors that are not only | ||
| # rational points (see trac #16225) | ||
| self._points = [(m, self.scheme().ambient_space().subscheme(p).rational_points()[0]) for (m, p) in self] | ||
| self._sort_points() |
There was a problem hiding this comment.
Wouldn't it be better to call this in __init__?
There was a problem hiding this comment.
Just to be sure: are you suggesting to move both lines 437 and 438 at the end of __init__, to be done when know_points is False?
There was a problem hiding this comment.
I think I might be misreading the code. What I meant was that wouldn't it be better if the points were sorted when the divisor is constructed so we don't need to check for this later on? I see now that you do call _sort_points somewhere in __init__. So why does it get called again here?
I'm probably just misunderstanding something because the divisor code is generally not great and I'm getting confused.
There was a problem hiding this comment.
Yes I thought it was weird as well, I don't know why it was done that way. I just moved some code to __init__ so that it makes more sense, let's see if this breaks anything.
|
Thank you for the feedback! Yes, I agree that the implementation of divisors should be rewritten. Your other PR you liked made me realise how the underlying formal sum and the set of points of the divisors are not linked, this seems very unsatisfying to me. So yeah, this is just a quick and dirty fix. In fact, during SageDays 130 I was working with someone who wanted to implement AG codes on surfaces, so she needed to implement divisors on surfaces. The way it is now done in Sage was not satisfying for that purpose, and we just realised the road was long to implement everything. Currently, many things are done for curves, and with the language of function fields, which is not the best to generalise. |
|
Test failures seem irrelevant. Thank you for the contribution! |
|
Yay! Thanks for the review! |
sagemathgh-41794: Sort divisor points Fixes sagemath#41793. In the aforementioned bug, an error is raised when one wants to access to the coefficient of a divisor on a curve. This happens because the method to get this coefficient assumes that the list is sorted, but it is not always the case. It turns out that not sorting the list of points led to other unwanted behaviours, such as the following one: ``` F = GF(13) PP.<X,Y,Z> = PolynomialRing(F,3) F = Y^2*Z - X^3 - X*Z^2 C = Curve(F) Points = C.rational_points() G = C.divisor([(1, Points[0]), (3,Points[0])]) print(G.coefficient(Points[0])) # Prints 1 ``` This PR fixes all of these bugs, assuming the set of points can be sorted. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation and checked the documentation preview. URL: sagemath#41794 Reported by: Rubén Muñoz--Bertrand Reviewer(s): Rubén Muñoz--Bertrand, Vincent Macri
sagemathgh-41794: Sort divisor points Fixes sagemath#41793. In the aforementioned bug, an error is raised when one wants to access to the coefficient of a divisor on a curve. This happens because the method to get this coefficient assumes that the list is sorted, but it is not always the case. It turns out that not sorting the list of points led to other unwanted behaviours, such as the following one: ``` F = GF(13) PP.<X,Y,Z> = PolynomialRing(F,3) F = Y^2*Z - X^3 - X*Z^2 C = Curve(F) Points = C.rational_points() G = C.divisor([(1, Points[0]), (3,Points[0])]) print(G.coefficient(Points[0])) # Prints 1 ``` This PR fixes all of these bugs, assuming the set of points can be sorted. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation and checked the documentation preview. URL: sagemath#41794 Reported by: Rubén Muñoz--Bertrand Reviewer(s): Rubén Muñoz--Bertrand, Vincent Macri
3 participants
Fixes #41793.
In the aforementioned bug, an error is raised when one wants to access to the coefficient of a divisor on a curve. This happens because the method to get this coefficient assumes that the list is sorted, but it is not always the case.
It turns out that not sorting the list of points led to other unwanted behaviours, such as the following one:
This PR fixes all of these bugs, assuming the set of points can be sorted.
📝 Checklist