Plane curves #2988
Plane curves #2988
Conversation
|
@HechtiDerLachs thanks for your support! |
| P in C && P in D || return 0 | ||
| S,_ = stalk(intersect(C,D), P) | ||
| return vector_space_dimension(S) |
There was a problem hiding this comment.
You need to check that the dimension is zero. The (possibly reducible) curves might have a common component passing through P. A return value of -1 for that case is not sufficiently clear for the user. An error message would be better.
| pa = zero(ZZ) | ||
| for i in 1:m | ||
| J = L[i][1].I | ||
| T, _ = grade(parent(J[1])) |
There was a problem hiding this comment.
Shouldn't normalization take care of grading in a reasonable way, if the input is graded?
There was a problem hiding this comment.
apparently normalization is not implemented for graded rings
| JJ = ideal(T, V) | ||
| B = quo(T, JJ) | ||
| H = hilbert_polynomial(B[1]) | ||
| pa = pa - ZZ(coeff(H, 0)) |
There was a problem hiding this comment.
Do we have something like constant_coefficient as a getter here to make this more readable?
|
@afkafkafk13 : A lot of the code is neither due to @simonbrandhorst , nor me. It's Delphine's old stuff that does not adhere to our coding conventions in many ways. However, the purpose of this PR is to integrate that into the schemes make it run somehow. I think nobody has the appetite to do further cosmetics at this point. If it works, it works. For some other parts of the code you are right with your suggestions and I will look into them. |
|
@HechtiDerLachs I am aware of this. Feel free to ignore the remarks which you consider inappropriate for the present pass through the code. I only typed up everything which caught my eye. |
|
I did what I could to fix up the creation of covered schemes from projective ones. Currently the tests still fail locally on my machine, but that seems to be due to non-visible methods of plane curves being tested. @simonbrandhorst : Can you take it again from here? |
|
I can confirm that all tests on the schemes are running again, except the newly added ones for curves. Hence, I return this PR to the hands of @simonbrandhorst again. |
|
Thank you! I will take it from here. |
Co-authored-by: Max Horn <max@quendi.de>
experimental/Experimental.jl
Outdated
| @@ -10,6 +10,7 @@ const oldexppkgs = [ | |||
| "ModStd", | |||
| "Rings", | |||
| "Schemes", | |||
| "PlaneCurve", | |||
There was a problem hiding this comment.
I will not be able to carry all the plane curve stuff over any time soon.
This disables the experimental package. Otherwise I get a lot of warnings of identifier conflics. (see below).
But disabling the package entirely seems drastic. @benlorenz do we have any other options?
Keep it in a module and not export anything?
WARNING: using PlaneCurveModule.invert_birational_map in module Oscar conflicts with an existing identifier.
WARNING: using PlaneCurveModule.rat_normal_curve_It_Proj_Even in module Oscar conflicts with an existing identifier.
... and so on
There was a problem hiding this comment.
Sorry about the delay, enabling the old code should work when removing/commenting the block at the bottom of the PlaneCurve.jl containing the using .PlaneCurveModule and the export statements. (It already lives in a submodule)
The difficult part would be getting the tests to run since we cannot do using Oscar and using Oscar.PlaneCurveModule because of the conflicting identifiers with the new code.
Maybe it would work to add a module around the tests:
module PlaneCurveTest
using ..Oscar
using ..Test
import Oscar.PlaneCurveModule
const PC = Oscar.PlaneCurveModule
...
end
And then one needs to change all calls that use functions from the module to PC.functionname.
|
@jankoboehm @wdecker I added plane curves to the documentation and adapted some names to Oscar conventions. For instance @fieker I put the experimental |
|
Something seems to be going seriously wrong with your reenabling of the old plane curve package. Your new package looks good to me, but I won't finish my review, before this problem is solved (one way or another). |
|
O.K. no clue how to include the plane curve package without breaking Oscar and the tests. |
|
Did you also disable the tests of the old package (experimental/PlaneCurve/test/runtests.jl), when disabling the package? |
|
If no one objects, I will merge this on Wednesday. |
|
No objections from my side, as soon as all required tests pass. |
|
The |
Good point. Maybe we should keep this in mind as an issue, but not block this PR by it? |
|
@wdecker Thanks for the remark. I will open an issue. |
|
@simonbrandhorst Thx, I agree with not blocking the PR at this point. |
This is a first attempt at bringing the experimental PlaneCurves module of Delphine Pol to src and make its terminology and interface consistent with the rest of the schemes.