-
Notifications
You must be signed in to change notification settings - Fork 129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Strict transform of an ideal in Cox ring #4154
base: master
Are you sure you want to change the base?
Strict transform of an ideal in Cox ring #4154
Conversation
TODO: tests, docstrings, documentation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4154 +/- ##
==========================================
- Coverage 84.58% 84.53% -0.05%
==========================================
Files 631 631
Lines 84999 85049 +50
==========================================
Hits 71899 71899
- Misses 13100 13150 +50
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I leave you a few comments on the code.
sideremark: I am not convinced (theorywise) that you can just set one entry to -1 in the grading change, but I am not a Cox-ring specialist. So I leave the reviewing of the content to someone else.
@assert grading_group(cox_ring(Y)) === class_group(Y) | ||
G = class_group(Y) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First define G as the class_group(Y) and then use G in the assert, since you need G afterwards anyway and in the case of failure of the assert, you did not loose anything.
n = number_of_generators(G) | ||
new_var = cox_ring(Y)[index_of_new_ray(f)] | ||
M = generator_degrees(cox_ring(Y)) | ||
@assert M[index_of_new_ray(f)][n] < 0 "Assuming the blowup adds a new column vector to `Oscar.generator_degrees(cox_ring(codomain(f)))`, the entry of that vector corresponding to the new ray should be negative. If not, multiply by -1." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your error messages in case of failing asserts seem pretty long to me. Please try to make them shorter and maybe add appropriate notes to the documentation instead.
for i in 1:n_rays(Y) | ||
i == index_of_new_ray(f) || @assert M[i][n] >= 0 "Assuming the blowup adds a new column vector to `Oscar.generator_degrees(cox_ring(codomain(f)))` for which the entry corresponding to the new ray is negative, the entries corresponding to all the other rays should be nonnegative. If not, add integer multiples of the other columns until it is." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to say
@assert findfirst(M[i][n] < 0 && i != index_of_new_ray(f) , 1:n_rays(Y)) === nothing
function strict_transform(f::ToricBlowupMorphism, I::MPolyIdeal) | ||
F, ind = _cox_ring_homomorphism(f) | ||
Y = domain(f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this!
This is to compute the strict transform of an ideal in the cox ring of the codomain, correct? I would probably insert an @assert
here, or something.
Once this is done, I would be curious to compare performance with the chart-based approach!
@paemurru : Just a question, because this has been sitting here for 2 weeks now. What is the status here? Are you making progress on this? Are you waiting for comments from someone else? If so, please ping this person. |
@afkafkafk13 I am working on this, this will probably stay as a draft pull request for quite some time. The current code only handles simple examples, the general case is a bit more complicated. The proof that it works uses a lot of notation, so is not suitable for a docstring or comment and must be typed in LaTeX. So it probably should be included in a research paper, unless an existing reference is found. Essentially, we have an abelian group homomorphism, in general not a C-algebra homomorphism and in general not unique, between the Cox rings that takes homogeneous ideals to homogeneous ideals. And the image of a homogeneous ideal is its total transform. |
TODO: tests, docstrings, documentation