-
Notifications
You must be signed in to change notification settings - Fork 240
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
Varieties v0.3 #3567
base: development
Are you sure you want to change the base?
Varieties v0.3 #3567
Conversation
@ggsmith @mikestillman is everyone happy with merging this? |
Also changed g \\ f to call quotient'(f, g) instead.
It would be good to have this merged soon so the branch is free to continue developing on. |
@mikestillman @ggsmith should I go ahead and merge this? We shouldn't wait until a week before the next release to merge big pull requests. |
I haven't yet had time to look at the details |
You had reviewed the previous PR #3517, which I accidentally closed and had to reopen. I think I've addressed all of your comments there. No other changes have been added since. Do you have an estimate of when you could look at the details? Varieties wasn't updated in version 1.24.05 and again didn't get updated in 1.24.11. There are new features and bug fixes that Devlin and I have worked on, but I'm afraid if we continue it won't make it to 1.25 either. |
@mahrud I'm ready to look at your few open PR's. I'm a bit confused about which to do in what order. 3522 first, then 3550, then 3567, then 3582? Are there duplicate commits in these? |
You can start from the smallest ones if you prefer, but the order they're reviewed or merged doesn't matter. |
@ggsmith @mikestillman Just wanted to check on this PR - it would be good to be able to continue developing the SheafMaps branch. If there's any changes that require explanation I'd be happy to discuss. |
pushout(Matrix, Matrix) := Module => (f, g) -> ( | ||
if source f =!= source g then error "expected maps with the same source"; | ||
h := f || -g; | ||
pushout List := Module => applyUniformMethod(symbol pushout, "pushout") |
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.
There is no documentation for new versions of pushout, pullback, pushoutMaps, pullbackMaps? It appears that there was no doc earlier too... But we should add it.
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.
Yes, there are lots of things not documented. (To be clear, pushout/pullback didn't exist in the Core before we added them for Varieties in the last PR)
@@ -28,6 +28,9 @@ isNormal = method() | |||
|
|||
normalCone = method(Options => true) | |||
|
|||
isMorphism = method(TypicalValue => Boolean) | |||
isAbelianCategory = method(TypicalValue => Boolean) |
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.
For Abelian categories, we need: coimageToImage map, and kernel and coker universal properties. I have code for that (at least for some categories, e.g. Modules, R-complexes), should we include it somewhere? Have you added it for sheaves, complexes of sheaves?
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.
It would be interesting, especially if this worked for other types of modules (e.g. quiver representations), but I was trying to do the minimal absolute necessary changes to get complexes of sheaves.
@@ -172,7 +190,7 @@ dim AffineVariety := X -> dim ring X | |||
dim ProjectiveVariety := X -> dim ring X - 1 -- TODO: - Picard rank instead? | |||
|
|||
char AffineVariety := X -> char ring X | |||
char ProjectiveVariety := X -> char(ring X / saturate ideal X) -- TODO: saturate with respect to B? | |||
char ProjectiveVariety := X -> char quotient saturate ideal X -- TODO: saturate with respect to B? | |||
|
|||
-- TODO: should these be defined, but return 0 for an AffineVariety? |
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.
Isn't there a well defined degree for an affine variety? The number of intersection points of a complementary dimensional affine linear space? This is computed as degree ideal leadTerm I, for the defining variety of I, as long as the monomial order is a degree order... It doesn't have as nice properties, but...
@@ -186,26 +204,23 @@ ambient AffineVariety := AffineVariety => X -> Spec ambient ring X | |||
ambient ProjectiveVariety := ProjectiveVariety => X -> Proj ambient ring X | |||
|
|||
-- arithmetic ops | |||
-- TODO: document | |||
AffineVariety ** AffineVariety := AffineVariety => (X, Y) -> Spec(ring X ** ring 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.
Even though these are affine varieties, the tensor product will create multi-degrees? That might not be desired...
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.
"Spec" will ignore the (multi)grading.
|
||
-- constructors | ||
sheaf = method() | ||
-- TODO: sheaf Ring and sheaf Module should return a sheaf over variety of the ring rather than Proj, | ||
-- and if a variety doesn't already exist then either Proj or Spec should be defined and cached. | ||
sheaf Ring := Ring ~ := SheafOfRings => R -> sheaf(Proj R, R) | ||
sheaf Ring := Ring^~ := SheafOfRings => R -> sheaf(variety R, R) |
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 this a breaking change?
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.
Not until we remove this in a future release:
-- TODO: remove by M2 1.25
tildeWarn = true
Thing~ := x -> (if tildeWarn then (tildeWarn = false; printerr "Note: M~ is deprecated; use M^~ or sheaf instead."); x^~)
|
||
--Comment about this code: isn't it sufficient to just check that the quotient by the | ||
--jacobian + the original ideal has finite length? Using the singularLocus command to do this | ||
--adds an additional saturation computation that seems unnecessary |
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!
@@ -13,7 +13,7 @@ document { | |||
Inputs => {"F" => {ofClass{CoherentSheaf,Module,Ideal} }}, | |||
Outputs => {ZZ }, | |||
"Computes the arithmetic genus of the coherent sheaf ", TT "F", " that is (-1)^dim-support * (chi(F) - 1)). If ", TT "F", " is a module | |||
over a ring, then the genus of ", TT "F~", " is computed. If ", TT "I", " is an ideal in a ring ", TT "R", " then the genus of ", TT "(R/I)~", " 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.
Maybe change to tex? Not for this PR maybe...
Description | ||
Text | ||
If either @TT "f"@ or @TT "g"@ is a ring element, then it is taken to be a | ||
scalar matrix acting on @TT "H"@. If @TT "g"@ is a @TO "MonomialIdeal"@, |
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.
What is H?
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.
Good question. This is identically moved here from the documentation of Matrix // Matrix. In particular, the typo is present in the current version.
g:{Matrix,MonomialIdeal,GroebnerBasis} | ||
Description | ||
Text | ||
If either @TT "f"@ or @TT "g"@ is a ring element, then it is taken to be a |
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.
Lately I have been preferring using
else error "expected a table of ring elements or matrices, or a list of elements of the same module") | ||
matrix List := Matrix => opts -> L -> ( | ||
if #L === 0 then return matrix(ZZ, {}); | ||
L = apply(splice L, splice); -- TODO: is this deepSplice? |
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 believe this is only "deepSplice" to 2 levels...
@mikestillman could you clarify which of your comments are things you want changed before this is merged and which are suggestions for future? Unless others want to contribute, the documentation suggestions are not going to happen in this PR. |
This is a repeat of #3517, which is on top of #3522 and shares a exact few commits with #3526, but I don't believe there should be a conflict.