[FTheoryTools] Unify blowup of singularities among toric varieties and schemes #3519

HereAround · 2024-03-14T22:38:02Z

src/Rings/MPolyMap/MPolyQuo.jl

HechtiDerLachs · 2024-03-18T18:51:20Z

I improvised a fix for the inclusion order similar to #3480 . Now the actual test failures are exposed.

@HereAround : I hope this was useful and you can take it from here?

HereAround · 2024-03-18T19:01:10Z

At the top of my head, I am not sure what caused the error no method matching keys(::Base.Generator{UnitRange{Int64}, Oscar.var"#7896#7897"{ZZMatrix}}). It strikes me as odd, but I will of course debug this and shall hope to eventually fix this.

fingolfin · 2024-03-20T10:37:51Z

What is the problem with the printing changes? Having a quick look, it seems that e.g.

│    Covering morphism
...
│    given by the pullback function
│      1a -> 3b
│        (x//z) -> 0

is changed to

│    Covering morphism
...
│    given by the pullback function
│      1a -> 3b
│        (x//z)

So the -> 0 is dropped? Maybe that's undesirable?

fingolfin · 2024-03-20T10:38:44Z

According to @afkafkafk13 this printing change is not desirable, so perhaps this part of the printing can be restored. Are there other issues? if so perhaps list them explicitly so we can discuss them as needed?

HereAround · 2024-03-20T11:53:44Z

According to @afkafkafk13 this printing change is not desirable, so perhaps this part of the printing can be restored. Are there other issues? if so perhaps list them explicitly so we can discuss them as needed?

Yes, I indeed feel so too.

This comes from an improvement that @HechtiDerLachs suggested. And as mentioned above by @thofma , it seems that the images of the generators are no longer reduced, and also made a suggestion for an attempt to fix it. I just pushed such a change - let us now wait to see if this reverts the printing of quotient rings to its original form.

... And this fix works. Great!

codecov · 2024-03-20T12:47:41Z

Codecov Report

Merging #3519 (edd844b) into master (657a0d1) will increase coverage by 0.02%.
Report is 7 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3519      +/-   ##
==========================================
+ Coverage   82.08%   82.10%   +0.02%     
==========================================
  Files         567      567              
  Lines       76652    76861     +209     
==========================================
+ Hits        62921    63109     +188     
- Misses      13731    13752      +21

Files	Coverage Δ
experimental/Schemes/CoveredProjectiveSchemes.jl	`88.40% <100.00%> (ø)`
...rimental/Schemes/ToricIdealSheaves/constructors.jl	`67.32% <100.00%> (+2.43%)`	⬆️
experimental/Schemes/Types.jl	`86.00% <ø> (ø)`
...ometry/Schemes/AffineSchemes/Objects/Properties.jl	`90.00% <ø> (-0.33%)`	⬇️
.../ToricVarieties/NormalToricVarieties/attributes.jl	`98.63% <100.00%> (+0.01%)`	⬆️
...etry/ToricVarieties/ToricLineBundles/attributes.jl	`96.77% <100.00%> (ø)`
src/Rings/MPolyMap/MPolyQuo.jl	`91.66% <100.00%> (+0.59%)`	⬆️
src/Rings/MPolyMap/MPolyRing.jl	`89.58% <100.00%> (+0.94%)`	⬆️

... and 14 files with indirect coverage changes

afkafkafk13 · 2024-03-20T19:11:20Z

src/Rings/MPolyMap/MPolyQuo.jl

+function _evaluate_plain(F::MPolyAnyMap{<:MPolyQuoRing, <:MPolyQuoRing}, u)
+  # This workaround deals with the fact that arithmetic in quotient rings is TERRIBLY slow.
+  # All the simplify calls make it unusable in this case, probably due to the fact that
+  # setting `is_reduced` flags does not pay off in such iterative procedures.
+  A = codomain(F)
+  v = evaluate(lift(u), lift.(_images(F)))
+  return simplify(A(v))
+end
+


No need to apologize for this in the comment! Arithmetic in quotients of polynomial rings is dead slow, when you reduce after each step. The art is to be rather lazy, not care about the choice of representative in complicated sequences of basic operations for a while and only reduce occasionally to normal form -- and of course whenever it is necessary to have a normal form as representative. This is precisely the philosophy you are pursuing here.

lgoettgens · 2024-03-27T11:52:17Z

@lgoettgens the tests here fail in the new Run CI without experimental / doctest job, inside the FTheoryTools tests, because combinations is not defined.
What confuses us is: why does it even run the doctests for FTheoryTools here?

Probably because of this line:

Oscar.jl/experimental/Experimental.jl

Line 62 in 890af7c

include("FTheoryTools/src/FTheoryTools.jl")

This, in combination with what I asked here breaks the no-experimental setup.

Yes, indeed.
We should get rid of the old-style experimental things at all (I.e. convert them). I think most should be just moving around some files. I can try to do this In a little disruptive way next week.
Mixing old and new styles as this is done in this PR will bite us in the long term and makes this already very finicky automatic inclusion even harder to maintain. (And it is completely orthogonal to the means of the new CI job)

HereAround · 2024-03-27T12:04:19Z

@lgoettgens the tests here fail in the new Run CI without experimental / doctest job, inside the FTheoryTools tests, because combinations is not defined.
What confuses us is: why does it even run the doctests for FTheoryTools here?

Probably because of this line:

Oscar.jl/experimental/Experimental.jl

Line 62 in 890af7c

include("FTheoryTools/src/FTheoryTools.jl")

This, in combination with what I asked here breaks the no-experimental setup.

Yes, indeed. We should get rid of the old-style experimental things at all (I.e. convert them). I think most should be just moving around some files. I can try to do this In a little disruptive way next week. Mixing old and new styles as this is done in this PR will bite us in the long term and makes this already very finicky automatic inclusion even harder to maintain. (And it is completely orthogonal to the means of the new CI job)

Most likely, it will be a lot of work to turn the schemes experimental folder into the new-style (if at all possible, with potential other PRs hanging.) Certainly would be great to have this, but maybe worth pinging @afkafkafk13 , @HechtiDerLachs and @simonbrandhorst, to hear if this is realistic.

afkafkafk13 · 2024-03-27T12:21:57Z

Yes, indeed. We should get rid of the old-style experimental things at all (I.e. convert them). I think most should be just moving around some files. I can try to do this In a little disruptive way next week. Mixing old and new styles as this is done in this PR will bite us in the long term and makes this already very finicky automatic inclusion even harder to maintain. (And it is completely orthogonal to the means of the new CI job)

Most likely, it will be a lot of work to turn the schemes experimental folder into the new-style (if at all possible, with potential other PRs hanging.) Certainly would be great to have this, but maybe worth pinging @afkafkafk13 , @HechtiDerLachs and @simonbrandhorst, to hear if this is realistic.

I agree that it is necessary and urgent to clean up experimental/Schemes, but from my point of view this should happen in a 2 step process:
a) Check which parts are sufficiently mature to go to src and move them
b) adjust the rest to the new style
Doing step b before step a will be extremely expensive, as a lot of files (and parts of files) will be moved around twice. this would also double the risks of this error-prone moving of stuff.

HereAround · 2024-03-27T13:33:14Z

Yes, indeed. We should get rid of the old-style experimental things at all (I.e. convert them). I think most should be just moving around some files. I can try to do this In a little disruptive way next week. Mixing old and new styles as this is done in this PR will bite us in the long term and makes this already very finicky automatic inclusion even harder to maintain. (And it is completely orthogonal to the means of the new CI job)

Most likely, it will be a lot of work to turn the schemes experimental folder into the new-style (if at all possible, with potential other PRs hanging.) Certainly would be great to have this, but maybe worth pinging @afkafkafk13 , @HechtiDerLachs and @simonbrandhorst, to hear if this is realistic.

I agree that it is necessary and urgent to clean up experimental/Schemes, but from my point of view this should happen in a 2 step process: a) Check which parts are sufficiently mature to go to src and move them b) adjust the rest to the new style Doing step b before step a will be extremely expensive, as a lot of files (and parts of files) will be moved around twice. this would also double the risks of this error-prone moving of stuff.

Just to clarify - this was about the style of the experimental code, not moving it into the OSCAR source.

From the above discussion (@lgoettgens and @benlorenz please correct if I am mistaken), I understood that this PR cannot be merged before experimental/Schemes complies with the new style of the experimental code, that @lkastner introduced a while back. This would mean to have folders:

experimental/Schemes/src , containing all the files currently in experimenal/Schemes,
experimental/Schemes/docs, with the corresponding documentation (currently presumably sprinkled into Oscar.jl/docs
experimental/Schemes/test, with the corresponding tests (also, unless I am mistaken, currently mixed in with Oscar.jl/test)
and a file experimental/Schemes/Schemes.jl, with the relevant include commands.

Copying the existing code from experimental/Schemes to a newly created experimental/Schemes/src is trivial, but disentangling the docs and tests probably a nightmare (please correct @afkafkafk13 if I am mistaken).

So I wonder what could be done to move this PR forward with the least effort on all parties involved.

afkafkafk13 · 2024-03-27T13:59:03Z

Of course, I am aware that moving stuff from Schemes to Schemes/src is not the problem ! This is the least of my worries.

But the docs and even more so some of the tests will not be a pleasant and safe task at all. Some other tests, on the other hand, are in separate files, which are again easy to move. But we might loose some intentional implicit testing of underlying functionality from src by means of these tests.

Luckily, quite a few recent PRs in the Schemes part have been merged over the last week, but @HechtiDerLachs has several older ones sitting around and I am not sure, what the status is for these and how badly each of these would be affected. I am not even sure, whether he might have already prepared to move some things to the src-directory of Oscar (this was, what I had alluded at). This needs to be discussed with @HechtiDerLachs as soon as he is back next week.

lgoettgens · 2024-03-27T14:15:08Z

Yes, indeed. We should get rid of the old-style experimental things at all (I.e. convert them). I think most should be just moving around some files. I can try to do this In a little disruptive way next week. Mixing old and new styles as this is done in this PR will bite us in the long term and makes this already very finicky automatic inclusion even harder to maintain. (And it is completely orthogonal to the means of the new CI job)

Most likely, it will be a lot of work to turn the schemes experimental folder into the new-style (if at all possible, with potential other PRs hanging.) Certainly would be great to have this, but maybe worth pinging @afkafkafk13 , @HechtiDerLachs and @simonbrandhorst, to hear if this is realistic.

I agree that it is necessary and urgent to clean up experimental/Schemes, but from my point of view this should happen in a 2 step process: a) Check which parts are sufficiently mature to go to src and move them b) adjust the rest to the new style Doing step b before step a will be extremely expensive, as a lot of files (and parts of files) will be moved around twice. this would also double the risks of this error-prone moving of stuff.

Just to clarify - this was about the style of the experimental code, not moving it into the OSCAR source.

From the above discussion (@lgoettgens and @benlorenz please correct if I am mistaken), I understood that this PR cannot be merged before experimental/Schemes complies with the new style of the experimental code, that @lkastner introduced a while back. This would mean to have folders:

experimental/Schemes/src , containing all the files currently in experimenal/Schemes,

experimental/Schemes/docs, with the corresponding documentation (currently presumably sprinkled into Oscar.jl/docs

experimental/Schemes/test, with the corresponding tests (also, unless I am mistaken, currently mixed in with Oscar.jl/test)

and a file experimental/Schemes/Schemes.jl, with the relevant include commands.

Copying the existing code from experimental/Schemes to a newly created experimental/Schemes/src is trivial, but disentangling the docs and tests probably a nightmare (please correct @afkafkafk13 if I am mistaken).

So I wonder what could be done to move this PR forward with the least effort on all parties involved.

I think it might be possible to just move the source for now, and postpone tests and docs for now. That's what I meant with little disruptive.
I can coordinate this next week during my visit in KL with @HechtiDerLachs and @HereAround.

HereAround · 2024-03-27T14:32:37Z

Yes, indeed. We should get rid of the old-style experimental things at all (I.e. convert them). I think most should be just moving around some files. I can try to do this In a little disruptive way next week. Mixing old and new styles as this is done in this PR will bite us in the long term and makes this already very finicky automatic inclusion even harder to maintain. (And it is completely orthogonal to the means of the new CI job)

Most likely, it will be a lot of work to turn the schemes experimental folder into the new-style (if at all possible, with potential other PRs hanging.) Certainly would be great to have this, but maybe worth pinging @afkafkafk13 , @HechtiDerLachs and @simonbrandhorst, to hear if this is realistic.

I agree that it is necessary and urgent to clean up experimental/Schemes, but from my point of view this should happen in a 2 step process: a) Check which parts are sufficiently mature to go to src and move them b) adjust the rest to the new style Doing step b before step a will be extremely expensive, as a lot of files (and parts of files) will be moved around twice. this would also double the risks of this error-prone moving of stuff.

Just to clarify - this was about the style of the experimental code, not moving it into the OSCAR source.
From the above discussion (@lgoettgens and @benlorenz please correct if I am mistaken), I understood that this PR cannot be merged before experimental/Schemes complies with the new style of the experimental code, that @lkastner introduced a while back. This would mean to have folders:

experimental/Schemes/src , containing all the files currently in experimenal/Schemes,

experimental/Schemes/docs, with the corresponding documentation (currently presumably sprinkled into Oscar.jl/docs

experimental/Schemes/test, with the corresponding tests (also, unless I am mistaken, currently mixed in with Oscar.jl/test)

and a file experimental/Schemes/Schemes.jl, with the relevant include commands.

Copying the existing code from experimental/Schemes to a newly created experimental/Schemes/src is trivial, but disentangling the docs and tests probably a nightmare (please correct @afkafkafk13 if I am mistaken).
So I wonder what could be done to move this PR forward with the least effort on all parties involved.

I think it might be possible to just move the source for now, and postpone tests and docs for now. That's what I meant with little disruptive. I can coordinate this next week during my visit in KL with @HechtiDerLachs and @HereAround.

Yeah, I was also starting to hope that we could get away by just taking care of the source. If successful, this is probably the way to go here.

(I was admittedly hoping to have this PR merged today, or at least this week. But then, I guess it can wait until next week. @apturner please let me know your feedback on the FTheoryTools changes, so I can try to work them in before we face up to the challenges with experimenal/Schemes next week).

Indeed, let us discuss this next week in KL.

HereAround · 2024-03-27T14:32:51Z

(Reverted to draft for the time being...)

-> Repeated execution will then lead to different sections, as desired

As suggested by Lars Kastner

HereAround · 2024-04-02T11:41:40Z

@HechtiDerLachs suggested an easier workaround to make us all happy, namely we "downgrade" (even though we keep the structure) FTheoryTools, in the sense that it will be included as an old experimental package. Thereby it can be loaded after the schemes, without the heroic effort of aligning the experimental schemes code to the new experimental standard.

lgoettgens · 2024-04-02T11:43:38Z

@HechtiDerLachs suggested an easier workaround to make us all happy, namely we "downgrade" (even though we keep the structure) FTheoryTools, in the sense that it will be included as an old experimental package. Thereby it can be loaded after the schemes, without the heroic effort of aligning the experimental schemes code to the new experimental standard.

This looks like a good idea for the meantime, in particular, to not block this PR.

experimental/Experimental.jl

simonbrandhorst · 2024-04-02T14:37:43Z

experimental/Schemes/CoveredProjectiveSchemes.jl

-      base_scheme(P) in patches(C) || error("base scheme not found in covering")
+      any(x->x===base_scheme(P), patches(C)) || error("base scheme not found in covering")


is this a == vs === issue? Looks like an easy mistake.

HereAround marked this pull request as draft March 14, 2024 22:38

HereAround added enhancement New feature or request topic: FTheoryTools labels Mar 14, 2024

HereAround changed the title ~~[FTheoryTools] Unify blowup of resolution among toric varieties and schemes~~ [FTheoryTools] Unify blowup of singularities among toric varieties and schemes Mar 15, 2024

HechtiDerLachs reviewed Mar 15, 2024

View reviewed changes

src/Rings/MPolyMap/MPolyQuo.jl Outdated Show resolved Hide resolved

HereAround force-pushed the RemoveRoadbloackInFTheoryTools branch 2 times, most recently from 670ca5b to 4d872a5 Compare March 18, 2024 17:16

HereAround force-pushed the RemoveRoadbloackInFTheoryTools branch 2 times, most recently from 1ba9ee5 to 098472a Compare March 19, 2024 21:29

HereAround force-pushed the RemoveRoadbloackInFTheoryTools branch from d796e4c to f49a1cc Compare March 20, 2024 11:57

HereAround force-pushed the RemoveRoadbloackInFTheoryTools branch from f49a1cc to 36c1ccb Compare March 20, 2024 15:58

afkafkafk13 reviewed Mar 20, 2024

View reviewed changes

HereAround force-pushed the RemoveRoadbloackInFTheoryTools branch 12 times, most recently from cfd1760 to 890af7c Compare March 27, 2024 06:39

HereAround marked this pull request as draft March 27, 2024 14:32

HereAround and others added 8 commits April 2, 2024 12:40

[ToricVarieties] Do not cache generic_section of toric line bundle

b3fbe8d

-> Repeated execution will then lead to different sections, as desired

[ToricVarieties] Fix to toric ideal (4ti2 error)

4100c89

As suggested by Lars Kastner

Avoid terribly slow evaluations in quotient rings.

e94edc2

[ToricSchemes] Adapt signatures to comparison with toric specs.

256973f

Make ideal sheaf attributable.

bc3eca3

Embarrassing fix in CoveredProjectiveScheme.

5835d13

[ToricSchemes] Improve and extend computation of toric ideal sheaves

d6292f8

[ToricVarieites] Combine test_files toric_blowups and toric_blowdowns

4d54161

HereAround force-pushed the RemoveRoadbloackInFTheoryTools branch from 890af7c to 8de6467 Compare April 2, 2024 10:40

HereAround marked this pull request as ready for review April 2, 2024 11:40

HechtiDerLachs approved these changes Apr 2, 2024

View reviewed changes

lgoettgens reviewed Apr 2, 2024

View reviewed changes

experimental/Experimental.jl Outdated Show resolved Hide resolved

[FTheoryTools] Extend resolve method to non-toric blowups

6901954

HereAround force-pushed the RemoveRoadbloackInFTheoryTools branch from 2c46e39 to 6901954 Compare April 2, 2024 12:13

HereAround enabled auto-merge (rebase) April 2, 2024 12:13

HereAround merged commit 92849f8 into oscar-system:master Apr 2, 2024
27 checks passed

simonbrandhorst reviewed Apr 2, 2024

View reviewed changes

HereAround mentioned this pull request Apr 2, 2024

FTheoryTools - Goals for 2024 and beyond #2699

Open

64 tasks

HereAround mentioned this pull request Apr 10, 2024

Backports Oscar 1.0.1 #3574

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FTheoryTools] Unify blowup of singularities among toric varieties and schemes #3519

[FTheoryTools] Unify blowup of singularities among toric varieties and schemes #3519

HereAround commented Mar 14, 2024

HechtiDerLachs commented Mar 18, 2024

HereAround commented Mar 18, 2024

fingolfin commented Mar 20, 2024

fingolfin commented Mar 20, 2024

HereAround commented Mar 20, 2024 •

edited

Loading

codecov bot commented Mar 20, 2024 •

edited

Loading

afkafkafk13 Mar 20, 2024

lgoettgens commented Mar 27, 2024

HereAround commented Mar 27, 2024

afkafkafk13 commented Mar 27, 2024

HereAround commented Mar 27, 2024

afkafkafk13 commented Mar 27, 2024

lgoettgens commented Mar 27, 2024

HereAround commented Mar 27, 2024

HereAround commented Mar 27, 2024

HereAround commented Apr 2, 2024

lgoettgens commented Apr 2, 2024

simonbrandhorst Apr 2, 2024

		base_scheme(P) in patches(C) \|\| error("base scheme not found in covering")
		any(x->x===base_scheme(P), patches(C)) \|\| error("base scheme not found in covering")

[FTheoryTools] Unify blowup of singularities among toric varieties and schemes #3519

[FTheoryTools] Unify blowup of singularities among toric varieties and schemes #3519

Conversation

HereAround commented Mar 14, 2024

HechtiDerLachs commented Mar 18, 2024

HereAround commented Mar 18, 2024

fingolfin commented Mar 20, 2024

fingolfin commented Mar 20, 2024

HereAround commented Mar 20, 2024 • edited Loading

codecov bot commented Mar 20, 2024 • edited Loading

Codecov Report

afkafkafk13 Mar 20, 2024

Choose a reason for hiding this comment

lgoettgens commented Mar 27, 2024

HereAround commented Mar 27, 2024

afkafkafk13 commented Mar 27, 2024

HereAround commented Mar 27, 2024

afkafkafk13 commented Mar 27, 2024

lgoettgens commented Mar 27, 2024

HereAround commented Mar 27, 2024

HereAround commented Mar 27, 2024

HereAround commented Apr 2, 2024

lgoettgens commented Apr 2, 2024

simonbrandhorst Apr 2, 2024

Choose a reason for hiding this comment

HereAround commented Mar 20, 2024 •

edited

Loading

codecov bot commented Mar 20, 2024 •

edited

Loading