-
Notifications
You must be signed in to change notification settings - Fork 133
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
Introduce is_known
according to #4394.
#4507
base: master
Are you sure you want to change the base?
Introduce is_known
according to #4394.
#4507
Conversation
@@ -240,3 +240,10 @@ function groebner_basis(I::MPolyIdeal; ordering::MonomialOrdering = default_orde | |||
is_global(ordering) || error("Ordering must be global") | |||
return standard_basis(I, ordering=ordering, complete_reduction=complete_reduction, algorithm=algorithm) | |||
end | |||
|
|||
function is_known(I::MPolyIdeal, ::typeof(groebner_basis); | |||
ordering::Union{MonomialOrdering, Nothing}=nothing) |
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.
Could/should this also accept the complete_reduction
kwarg?
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.
That depends on the user's need. In principle, I think, it could be added (if we agree on supporting forwarding of kwargs for is_known
). But as this is just a demonstration right now, I restricted to this signature for the time being.
return ideal(base_ring(I), unique!(newgens)) | ||
result = ideal(base_ring(I), unique!(newgens)) | ||
# carry over eventual information about the dimension | ||
is_known(I, dim) && is_known(J, dim) && (result.dim = maximum([dim(I), dim(J)])) |
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.
Matter of taste, but I personally think assignments in a chain of &&
takes it a step to far in terms of how it affects code clarity. But feel free to ignore this suggestion shrug
is_known(I, dim) && is_known(J, dim) && (result.dim = maximum([dim(I), dim(J)])) | |
if is_known(I, dim) && is_known(J, dim) | |
result.dim = maximum([dim(I), dim(J)])) | |
end |
get_attribute(I, :is_prime, false) && return true | ||
return false | ||
function is_known(I::MPolyIdeal, ::typeof(is_radical)) | ||
is_known(I, is_prime) && is_prime(I) && return true |
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.
This is starting to look like a reinvention of GAP's property implications (albeit manually and much less powerful -- no offense intended). See also issue #982 by @HereAround
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.
Maybe, but I don't know too much about the GAP internals, to be honest. @HereAround 's request seems related indeed. Again, for the moment this is thought of as a demonstration of potential use cases for is_known
. I think it can well be integrated in any other (more powerful) logical deduction system, can it not? It's the programmer's liberty how to make use of it. This is just to say: Look, this is what you can do for example.
D = minimal_primes(I) | ||
return length(D) == 1 && issubset(D[1], I) | ||
end | ||
|
||
function is_known(I::MPolyIdeal, ::typeof(is_prime)) | ||
has_attribute(I, :is_prime) && return true | ||
is_known(I, primary_decomposition) && return true |
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.
Shouldn't this be
is_known(I, primary_decomposition) && return true | |
is_known(I, primary_decomposition) && is_one(length(primary_decomposition(I))) && return true |
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.
No, I don't think so. The point of is_known
is whether or not the result can be computed in constant time. Once the primary decomposition is there, I think it takes (almost) constant time to check whether its length is one and whether the single component P
is a subset of I
.
Well, now that I think of it again, this should probably also involve an is_known(I, groebner_basis)
. But I think your particular suggestion is not correct.
has_attribute(I, :is_prime) && return true | ||
is_known(I, primary_decomposition) && return true | ||
is_known(I, minimal_primes) && return true | ||
if coefficient_ring(base_ring(I)) isa Field |
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.
Wouldn't it suffice if the base_ring is a domain, as echeck via is_domain_type
? I.e.
if coefficient_ring(base_ring(I)) isa Field | |
if is_domain_type(base_ring_type(I)) |
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.
You are probably right. This might be a reasonable extension. Just didn't occur to me when I wrote it up.
return true | ||
end | ||
|
||
is_known(I, is_prime) && is_prime(I) && return true | ||
D = primary_decomposition(I) | ||
return length(D) == 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.
Should there also be a is_known(I::MPolyIdeal, ::typeof(is_primary))
method?
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.
If we have that function, then yes: Extending the is_known
functionality for that seems reasonable. But again: This PR is just to demonstrate some applications of is_known
so that we can all contemplate where the journey goes. I do not make any claim about coverage of the subject on mpoly-ideals
or elsewhere. So far, one of our decisions is that we do not guarantee support for is_known
just by introduction of a method of a unary function for an object. So in cases like this, functionality can be extended at need and request.
Still waiting for a PR in AA to first be discussed.