-
Notifications
You must be signed in to change notification settings - Fork 112
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
Use proper morphisms in primary decomposition helpers #3109
Conversation
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.
Much appreciated, thanks!
@@ -71,7 +68,8 @@ function _expand_coefficient_field( | |||
theta = gens(A_exp)[1:r] | |||
alpha = gens(coefficient_ring(A)) | |||
to_A = hom(A_exp, A, vcat(A.(alpha), gens(A))) | |||
to_A_exp = hom(A, A_exp, x->evaluate(x.data, theta), gens(A_exp)[r+1:end]) | |||
#to_A_exp = hom(A, A_exp, x->evaluate(x.data, theta), gens(A_exp)[r+1:end]) |
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.
Is there a reason why you wanted to keep this specific line? I.e. any doubts here?
Also: Does your new hom
have a check
flag which we could/should set to false
in all these calls?
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.
Oops, just an artifact. Yes, there is a check
argument, so we "could". I can enable it if you want. I have make another commit anyway.
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.
Alright. Whether we should depends on how expensive the check is. Since the necessary Groebner bases computations here are carried out automatically with the creation of the quotient rings anyway, I think it's OK to leave it as 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.
I agree. If it turns out to be a bottleneck, we can add it later.
No description provided.