Basic functionalities for weighted projective {curves, points, spaces}

[user202729]

Picking up #40070

as far as I can tell the fedora:42 failure is irrelevant.

📝 Checklist

• The title is concise and informative.
• The description explains in detail what this PR is about.
• I have linked a relevant issue or discussion.
• I have created tests covering the changes.
• I have updated the documentation and checked the documentation preview.

⌛ Dependencies

[github-actions]

Documentation preview for this PR (built with commit 8c4f225; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

[vincentmacri]

Looks good overall, just a bunch of nitpicks.

Tagging @GiacomoPope @grhkm21 who were involved in the previous PR.

[vincentmacri]

It would be nice to include an example of this in the docstring.

[user202729]

actually I find this mathematically questionable. Map of rings induce map of their scheme in opposite direction, right.

[vincentmacri]

actually I find this mathematically questionable. Map of rings induce map of their scheme in opposite direction, right.

Honestly not sure, I don't use the scheme approach to this topic myself. This code does seem to match with projective_space.py though. Either way, I think this is more of a convenience thing, so it should be fine as long as the behaviour is clearly documented. To that effect, could you add a note to the INPUT section of the docstring saying that if R is a morphism then we return a weighted projective space over the codomain of the morphism?

[user202729]

Someone else may want to double check the equality comparison logic in case of rings with zero divisor (I haven't).

[vincentmacri]

Remove redundant assert. Other than the integral domain issue I think this looks good.

[vincentmacri]

actually I find this mathematically questionable. Map of rings induce map of their scheme in opposite direction, right.

Honestly not sure, I don't use the scheme approach to this topic myself. This code does seem to match with projective_space.py though. Either way, I think this is more of a convenience thing, so it should be fine as long as the behaviour is clearly documented. To that effect, could you add a note to the INPUT section of the docstring saying that if R is a morphism then we return a weighted projective space over the codomain of the morphism?

[vincentmacri]

It might exist if c2 is a unit. Or it raises an error, which is bad. We could catch the error, but then what do we return if we are unable to determine if the elements are equal or not?

[vincentmacri]

Someone else may want to double check the equality comparison logic in case of rings with zero divisor (I haven't).

Doesn't the way it's implemented actually require it to be a field (since it assume we can divide)? Maybe _richcmp_ should just return/raise NotImplemented/NotImplementedError or whatever the default is for the non-field case.

[user202729]

not really, you can divide in integral domain (and even Zmod) provided its fraction field is implemented.

anyway not like it's my code. I'd struggle to figure out whether $ℤ[x]/(2x-3)$ is supposed to be isomorphic to $ℤ$ (it isn't) (wait, then how is $(2 : 3)$ a point in $ℙ_ℤ^2$? I thought we only consider $ℤ$-valued points?)

[vincentmacri]

not really, you can divide in integral domain (and even Zmod) provided its fraction field is implemented.

Okay, and I guess the coercion system handles this?

I'd struggle to figure out whether $ℤ[x]/(2x-3)$ is supposed to be isomorphic to $ℤ$ (it isn't) (wait, then how is $(2 : 3)$ a point in $ℙ_ℤ^2$? I thought we only consider $ℤ$-valued points?)

I'll need to think about this, but I'm pretty busy this week. I'll try to get back to you on Friday.

[user202729]

Okay, and I guess the coercion system handles this?

pretty much. There's a lot of (possibly slow…) magic going on under the hood.

[edit] not really. The return value is whatever _div_ returns. The coercion system only ensure that both sides of the binary operator have the same parent.

sometimes some classes override the coercion system and e.g. implement __truediv__ directly.

[vincentmacri]

I do not think that _richcmp_ as implemented works if R is not an integral domain, and I cannot think of a reliable way to handle the case where it is not an integral domain. Here is an example.

P = WeightedProjectiveSpace(Zmod(8), (1, 2))
a = P(4, 1)
b = P(0, 4)
print(a == b)  # Returns False, but these are equivalent

Note that over $\mathbb{Z} / 8\mathbb{Z}$ with $\lambda = 2$ we have $b \equiv a$ because $(4 \cdot \lambda, 1 \cdot \lambda^2) = b$.

So I think _richcmp_ should raise some kind of error or return NotImplemented if R is not an integral domain, unless you have an idea for a different implementation of _richcmp_. Alternatively, I don't think the current implementation can incorrectly return True, it can only incorrectly return False. So maybe if R is an integral domain then anytime we would have returned that the elements are not equal, we instead raise an error or return NotImplemented, but if we conclude that the elements are equal then we still allow that to work.

I think I misunderstood the definition of projective space over non-fields. I'll double check

[vincentmacri]

I misunderstood the definition, $\lambda$ needs to be a unit, and 2 is not a unit mod 8. But here's another issue.

P = WeightedProjectiveSpace(Zmod(8), (1, 1))
P(4, 4)  # Works

P = ProjectiveSpace(Zmod(8), 1)  # Same as WeightedProjectiveSpace(Zmod(8), (1, 1))
P(4, 4)  # ValueError: [4, 4] does not define a valid projective point since it is a multiple of a zero divisor

so WeightedProjectiveSpace is allowing us to construct points that are not valid points.

[vincentmacri]

Let's not allow non-integral domains when check=True.

Otherwise this looks good.

[vincentmacri]

Suggested change

	R = X.value_ring()
	R = X.value_ring()
	if R not in IntegralDomains():
	raise ValueError("cannot check point over a ring that is not an integral domain")

[vincentmacri]

Suggested change

	# over other rings, we do not have a generic method to check
	# whether the given coordinates is a multiple of a zero divisor
	# so we just let it pass.

Don't let it pass, the user should have to specify check=False to allow something potentially invalid to happen.

[user202729]

possible, but the API would be excessively clumsy if you have to check=False the creation of every point.

[vincentmacri]

Perhaps, but we could always move the check option to the WeightedProjectiveSpace itself if that becomes an issue, and then it would just need to be given once. I doubt many users are likely to do significant computations over a ring that isn't an integral domain anyway. The main motivation for this PR was to use it for hyperelliptic Jacobian computations, which is done over fields anyway.

I'll take one last look at the code later today, but unless I find something I missed earlier I'll approve it. Thank you for picking this up!

[vincentmacri]

CI failures seem irrelevant. Looks good to me!

Thank you again for picking this up! Now we can move forward with #39161.