Skip to content
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

addresseing Issue #2427 #2473

Merged
merged 8 commits into from Jun 18, 2023
Merged

addresseing Issue #2427 #2473

merged 8 commits into from Jun 18, 2023

Conversation

wdecker
Copy link
Collaborator

@wdecker wdecker commented Jun 15, 2023

No description provided.

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks good to me, with the caveat that I have no idea what the change in the schemes code from isprime(D) to isone(D) || isprime(D) does and if it is the right thing to do.

Copy link
Collaborator

@HechtiDerLachs HechtiDerLachs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see whether this works.

experimental/Schemes/IdealSheaves.jl Show resolved Hide resolved
experimental/Schemes/IdealSheaves.jl Show resolved Hide resolved
experimental/Schemes/WeilDivisor.jl Outdated Show resolved Hide resolved
experimental/Schemes/WeilDivisor.jl Outdated Show resolved Hide resolved
wdecker and others added 4 commits June 16, 2023 11:57
Co-authored-by: Matthias Zach <85350711+HechtiDerLachs@users.noreply.github.com>
Co-authored-by: Matthias Zach <85350711+HechtiDerLachs@users.noreply.github.com>
Co-authored-by: Matthias Zach <85350711+HechtiDerLachs@users.noreply.github.com>
Co-authored-by: Matthias Zach <85350711+HechtiDerLachs@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Jun 16, 2023

Codecov Report

Merging #2473 (e90d80e) into master (cf57c0f) will decrease coverage by 0.02%.
The diff coverage is 84.61%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2473      +/-   ##
==========================================
- Coverage   73.12%   73.11%   -0.02%     
==========================================
  Files         400      400              
  Lines       53588    53607      +19     
==========================================
+ Hits        39188    39194       +6     
- Misses      14400    14413      +13     
Impacted Files Coverage Δ
src/Rings/mpoly-ideals.jl 84.34% <80.00%> (-0.56%) ⬇️
experimental/Schemes/IdealSheaves.jl 82.75% <100.00%> (+0.21%) ⬆️

... and 9 files with indirect coverage changes

@simonbrandhorst
Copy link
Collaborator

Looks good to me. Somewhat aside to this PR:

I think that is_prime(::IdealSheaf) does not do what it is supposed to. We only check that it is locally prime at some open cover. But I would say that is not a local property. Because we can take a disconnected scheme of two points {P1,P2} and take the open cover U1 = {P1} and U2={P2}. Now consider the zero sheaf of ideals.
It will be locally prime but not globally.
So what our function checks is just that a sheaf of prime ideals is locally prime. But we do not check that it is prime for every affine open (e.g. the affine open containing both {P1,P2}). And even if we could, I am not sure if we still get something irreducible in the non-separated case.

In any case this behavior is unrelated to the PR and we should discuss it in an issue.

@afkafkafk13
Copy link
Collaborator

Looks good to me. Somewhat aside to this PR:

I think that is_prime(::IdealSheaf) does not do what it is supposed to. We only check that it is locally prime at some open cover. But I would say that is not a local property. Because we can take a disconnected scheme of two points {P1,P2} and take the open cover U1 = {P1} and U2={P2}. Now consider the zero sheaf of ideals. It will be locally prime but not globally. So what our function checks is just that a sheaf of prime ideals is locally prime. But we do not check that it is prime for every affine open (e.g. the affine open containing both {P1,P2}). And even if we could, I am not sure if we still get something irreducible in the non-separated case.

In any case this behavior is unrelated to the PR and we should discuss it in an issue.

I agree and would like to add that is_connected is not yet implemented for schemes, whence the above issue is not one to be fixed within days.

@simonbrandhorst simonbrandhorst merged commit 04c90ae into master Jun 18, 2023
13 of 16 checks passed
@simonbrandhorst simonbrandhorst deleted the Wolfram branch June 18, 2023 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants