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

Varieties v0.3 #3517

Merged
merged 211 commits into from
Nov 1, 2024
Merged

Varieties v0.3 #3517

merged 211 commits into from
Nov 1, 2024

Conversation

mahrud
Copy link
Member

@mahrud mahrud commented Oct 15, 2024

This PR contains work throughout Spring and Summer 2024, including at the workshop at Utah. Complexes of sheaves are added, but are currently disabled until the Complexes package can be preloaded. Lots of work remains.

cc: @kellerlv @ggsmith @Devlin-Mallory @ritvikrr @johndcobb

@mahrud
Copy link
Member Author

mahrud commented Oct 15, 2024

I'm not sure if @mikestillman has forgotten about mikestillman#38, but this PR also includes those changes.

@mahrud
Copy link
Member Author

mahrud commented Oct 15, 2024

@d-torrance any idea what's happening here? I don't think this PR changes anything related to this ...

--*- compilation -*-

i1 : input "../../../../../Macaulay2/tests/normal/alarm.m2"

ii2 : -- no-check-flag (see https://github.com/Macaulay2/M2/issues/1392)
      -- test that alarms work
      
      time try ( alarm(1); while true do 1 ) else true
 -- used 1.13874s (cpu); 0.999088s (thread); 0s (gc)

oo2 = true

ii3 : 
      R = QQ[vars(0..24)]

oo3 = R

oo3 : PolynomialRing

ii4 : f = () -> (alarm 4; try res coker vars R else "ran out of time")

oo4 = f

oo4 : FunctionClosure

ii5 : time f()


 *** out of memory trying to allocate 128 bytes, exiting ***
Command exited with non-zero status 1
16.95user 4.55system 0:11.87elapsed 181%CPU (0avgtext+0avgdata 477640maxresident)k
0inputs+8outputs (0major+13659minor)pagefaults 0swaps

@d-torrance
Copy link
Member

Not sure. Apparently this test has been a problem before since we actually skip it when when running check Core (see #1867).

@d-torrance
Copy link
Member

@mahrud - Do you think it's worth applying the SumsOfSquares test fix on development separately while we wait for this PR to be review?

@mahrud
Copy link
Member Author

mahrud commented Oct 27, 2024

Do you think it's worth applying the SumsOfSquares test fix on development separately while we wait for this PR to be review?

I think you meant to comment this on #3519, where bbc9f09 is, no? I think it's good to keep it there as extra incentive for reviewing it.

@d-torrance
Copy link
Member

Sure -- it looks like this PR is on top of that one, so either way. :) Sounds good!

drop(ws := separate_" " s, -1),
if plurals#?(last ws)
then plurals#(last ws) else last ws | "s")
pluralsynonym = T -> if T.?synonym then pluralize T.synonym else "objects of class " | toString T
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these changes part of a pull request focusing on Varieties? I would have guessed that they were entirely unrelated.

Copy link
Member Author

@mahrud mahrud Oct 28, 2024

Choose a reason for hiding this comment

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

It's included to make the following error print correctly:

i3 : directSum(Proj QQ[x], Proj QQ[y])
stdio:2:9:(3): error: no method for direct sum of projective varieties

i5 : directSum(Spec QQ[x], Spec QQ[y])
stdio:4:9:(3): error: no method for direct sum of affine varieties

inducedMap(P, T) * map(T, target g, T_[1], Degree => - degree g)};
P.cache.formation = FunctionApplication (pushout, args);
P.cache.pushoutMaps = apply(#args,
i -> inducedMap(P, T) * map(T, target args#i, T_[i], Degree => - degree args#i));
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the pullback and pushout documented or tested anywhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

They are tested in Varieties, but the documentation is only a stump.

@@ -64,7 +64,7 @@ placeholderToSchubertBasis(RingElement,FlagBundle) := (c,G) -> (
(k,q) := toSequence(G.BundleRanks);
P := diagrams(q,k);
M := apply(P, i-> placeholderSchubertCycle(i,G));
E := flatten entries basis(R);
E := flatten entries basis(R, Variables => 0 .. numgens R - 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to depend on other changes to basis, which I am personally not yet sold on.

@@ -203,7 +203,7 @@ getBasis (ZZ,DGAlgebra) := opts -> (homDegree,A) -> getBasis(homDegree,A.natural
getBasis (ZZ,Ring) := opts -> (homDegree,R) -> (
local retVal;
myMap := map(R, R.cache.basisAlgebra);
tempList := (flatten entries basis(homDegree, R.cache.basisAlgebra, Limit => opts.Limit)) / myMap;
tempList := (flatten entries basis(homDegree, R.cache.basisAlgebra, Limit => opts.Limit, Variables => 0 .. numgens R-1)) / myMap;
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to depend on other changes to basis?

-- TODO: should matrix RingMap return this map instead?
-- mon := map(module T, image matrix(S, { S.FlatMonoid_* }), f, matrix f)
mat := last coefficients(f cover srcgens, Monomials => targens);
map(image targens, image srcgens, f, mat))
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't these many changes to basis already part of an independent pull request?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but this PR is on top of that because of actual dependencies (but it is not on top of the part PR).

@@ -177,7 +177,7 @@ nefGenerators NormalToricVariety := Matrix => X -> (
fromPic := matrix fromPicToCl X;
indexOfPic := abs lcm (minors( rank source fromPic, fromPic^torsionlessCoord))_*;
(indexOfPic * coneGens) // fromPic
);
))
Copy link
Contributor

Choose a reason for hiding this comment

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

By definition, the nef cone lives in the Neron--Severi group (which for a toric variety is canonical isomorphic to the Picard group). Does returning something in the class group make mathematical sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I don't follow. The only change here is caching of nefGenerators. What change are you referring to?

Copy link
Contributor

@ggsmith ggsmith left a comment

Choose a reason for hiding this comment

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

Can this pull request be decoupled from the changes to basis? Can the changes to quotient modules and matrices to extracted?

@mahrud
Copy link
Member Author

mahrud commented Oct 28, 2024

Can this pull request be decoupled from the changes to basis?

It is decoupled. I don't remember exactly why, but the changes to basis stemmed from fixing things in Varieties, which is why this PR is on top of that one.

Can the changes to quotient modules and matrices to extracted?

I tried to do this a while ago but I'm afraid it was very complicated (random tests kept failing in one package or another when I tried to separate them) and I gave up, so I don't want to do it. If you or someone else wants to give it an attempt you're welcome to.

@mahrud
Copy link
Member Author

mahrud commented Oct 29, 2024

Okay, updates:

  • Opened Implement left and right quotients via \\ and // #3550 for quotient changes (though this PR is still on top of it), I think the errors I couldn't fix last time were SumsOfSquares errors, I don't remember.
  • Fixed the error message for non-standard graded rings
  • Removed the Cartan-Eilenberg commit (it is safe on another branch for later)

Copy link
Member

@mikestillman mikestillman left a comment

Choose a reason for hiding this comment

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

Once @ggsmith is happy with this, it looks fine to me, with the caveat that I'm not so convinced that the breaking change to basis is worth the breaking. I think basis needs a complete revamping at some point, with more expressive names for what it is doing. But other than that, this all looks good. I did not go into all the Varieties code in detail though. I am happy to see complexes of sheaves though!

M2/Macaulay2/packages/Complexes.m2 Show resolved Hide resolved
M2/Macaulay2/packages/Varieties/backports.m2 Outdated Show resolved Hide resolved
@mahrud mahrud merged commit 229c6ef into Macaulay2:development Nov 1, 2024
5 checks passed
@mahrud
Copy link
Member Author

mahrud commented Nov 1, 2024

Argh this isn't merged, I just made a mistake. I'll reopen a new one.

@mahrud mahrud mentioned this pull request Nov 1, 2024
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.

8 participants