[FTheoryTools] Rewrite _kodaira_type to work with any locus #3424
Conversation
| quotient_gens = gens(groebner_basis(quotient(ideal([9 * poly_g]), ideal([2 * poly_f])) + locus_id)) | ||
| deleteat!(quotient_gens, findall(g -> g == locus, quotient_gens)) | ||
| quotient_val = quotient_gens[1] |
There was a problem hiding this comment.
I am wondering if the check g == locus will always work. Generators of the quotient are not unique. Will the Groebner basis computation completely eliminate this redundancy? Also, could additional signs/numeric factors appear that would force g == locus to return false?
Less critical, but relevant: Unless I am mistaken this pattern of lines appears four times in this PR. Does it make sense to add an auxiliary function and call it instead to make the code shorter/cleaner?
There was a problem hiding this comment.
Thank you @apturner . Generally speaking, looks good. I like the new tests.
I left a few nitpicks above, in case you want to address that. Otherwise, this can be merged from my point of view.
|
Thank you @apturner! |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #3424 +/- ##
==========================================
- Coverage 81.93% 81.46% -0.47%
==========================================
Files 561 563 +2
Lines 75170 80142 +4972
==========================================
+ Hits 61588 65289 +3701
- Misses 13582 14853 +1271
|
|
Sorry @apturner - I just wanted to rebase this PR to the latest master (was very outdated - apparently a lot of last minute changes for the OSCAR 1.0 release over the last two weeks), so that I could convince myself that this PR works with all those changes. Usually, the rebase is pretty easy (for me, even more so if it is my own PR), but in this particular case something malfunctioned... Sorry about that. In any case: I just opened a new PR with your changes here: #3506. |
|
I rolled back the two additional changes mentioned in #3506, as they are causing tests to fail for an unrelated reason. I will add these changes to the TODO. |
Rewrite the function
_kodaira_typeso that it should now work with any locus, rather than just loci of the formw = 0forwa coordinate.