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

Add mres and nres Singular Calls for Free resolutions #2500

Merged
merged 5 commits into from Sep 18, 2023

Conversation

RafaelDavidMohr
Copy link
Contributor

Adds corresponding keywords to free_resolution and adjusts docu accordingly.

Comment on lines 6363 to 6364
elseif algorithm == :sres
res = Singular.fres(singular_kernel_entry, length)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There seems to be an old bug: you want sres but ask for fres!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in d6abd9f

@wdecker
Copy link
Collaborator

wdecker commented Jun 29, 2023

Once this is merged, I will extend the docu and explain what the difference between the different methods is.

Project.toml Outdated
@@ -35,7 +35,7 @@ Polymake = "0.10.0"
Preferences = "1"
RandomExtensions = "0.4.3"
RecipesBase = "1.2.1"
Singular = "0.18.2"
Singular = "0.18.6"
Copy link
Member

Choose a reason for hiding this comment

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

This looks right (I didn't check the versions, but I trust you did).

Thanks!

@fingolfin
Copy link
Member

This PR has no test for the new code, so naturally it will pass, even if actually calling the code was broken.

So either a test should be added in this PR (could be a doctest or could be a regular test); or else I'd hope that @wdecker would add one when he extends the documentation for this?

@RafaelDavidMohr
Copy link
Contributor Author

A word on the now failing tests: I could be wrong, but to me it seems that the issue is the following:

Oscar starts computing a free resolution by computing the input module as a cokernel and then passes that cokernel to Singular to let it compute a free resolution for that. Singular.sres returns this free resolution but with the generators of this cokernel potentially permuted. Now the presentation of the input module and the Singular resolution don't "patch together" correctly and thus give a wrong free resolution in Oscar.

My question is: Should we try to handle this bug in this PR or a seperate one?

Copy link
Collaborator

@wdecker wdecker left a comment

Choose a reason for hiding this comment

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

There are several problems here: To begin with, fres and sres require as input a gröbner basis. This is missing so far. There is another problem on which @jankoboehm is already working.

For the time being, I suggest to replace sres by fres in the test. If this makes the test pass, let us merge the PR. More tests and docu will be added as soon as all problems are fixed. @RafaelDavidMohr please open an issue which reminds us to do the above.

@RafaelDavidMohr
Copy link
Contributor Author

Unfortunately the same problem seems to be happening with mres: Singular changes the cokernel representation of the module and then the free resolution Singular gives doesn't glue correctly with what is computed on the Oscar side.

So mres would require similar changes to the free resolution code that sres needs.

@wdecker
Copy link
Collaborator

wdecker commented Jun 30, 2023

The situation for mres is different and more subtle as mres computes a new presentation. In particular, this requires that we return an isomorphism between the given module and the new module. That means that we have to introduce some book-keeping in Singular and add the result of this to the output of mres. So let me suggest to make this PR a draft until all these prblems have been addressed.

@RafaelDavidMohr
Copy link
Contributor Author

The situation for mres is different and more subtle as mres computes a new presentation. In particular, this requires that we return an isomorphism between the given module and the new module. That means that we have to introduce some book-keeping in Singular and add the result of this to the output of mres. So let me suggest to make this PR a draft until all these prblems have been addressed.

Ok! In a sence, sres also computes a new representation (but just by shuffling around generators).

Let me know if I can help with fixing this.

@RafaelDavidMohr RafaelDavidMohr marked this pull request as draft June 30, 2023 10:19
@fingolfin
Copy link
Member

We think there are some changes in the latest Singular.jl that may help, but couldn't find them just now; but @hannes14 will check

@RafaelDavidMohr
Copy link
Contributor Author

I added mres, the bug that I originally described (with Singular changing the presentation of some module) looks to be actually easily fixed by some global changes to the free_resolution function, we just have to also take the first map that Singular gives us.

Let me know if I missed something.

@ederc
Copy link
Member

ederc commented Sep 7, 2023

Looks like a valid solution to me.

@RafaelDavidMohr RafaelDavidMohr marked this pull request as ready for review September 8, 2023 09:10
@fingolfin
Copy link
Member

If the geometry would like to merge this, fine by me, but someone needs to explicitly approve it

@wdecker
Copy link
Collaborator

wdecker commented Sep 11, 2023 via email

@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Merging #2500 (347349c) into master (79874fd) will increase coverage by 0.72%.
Report is 24 commits behind head on master.
The diff coverage is 74.80%.

@@            Coverage Diff             @@
##           master    #2500      +/-   ##
==========================================
+ Coverage   73.63%   74.36%   +0.72%     
==========================================
  Files         455      455              
  Lines       64411    68587    +4176     
==========================================
+ Hits        47427    51002    +3575     
- Misses      16984    17585     +601     
Files Changed Coverage
experimental/JuLie/src/schur_polynomials.jl ø
...ntal/QuadFormAndIsom/src/lattices_with_isometry.jl ø
experimental/Schemes/CoveredProjectiveSchemes.jl ø
src/AlgebraicGeometry/Schemes/Glueing/Methods.jl ø
...eometry/Schemes/ProjectiveSchemes/Objects/Types.jl ø
src/Modules/FreeModules-graded.jl 0.00%
src/Rings/mpoly-affine-algebras.jl 0.00%
src/Rings/mpoly-graded.jl 0.00%
src/TropicalGeometry/curve.jl 0.00%
src/Serialization/containers.jl 20.00%
... and 13 more

@RafaelDavidMohr
Copy link
Contributor Author

RafaelDavidMohr commented Sep 12, 2023

Here is a bit more explanation on why things should work now. Originally I diagnosed the problem wrong.

Previously, for a given module M, free_resolution(M) would perform the following computations:

We start by computing a presentation of M

0 <-- M <-- F <-- G

then we let C be the cokernel of this representation (i.e. the image of G in F) and let Singular compute a free resolution of this cokernel:

0 <-- F/C <-- F0 <-- F1 <-- F2 <-- ...

and now the two chain complexes were patched together as follows:

0 <-- M <-- F <-- G <-- F2 <-- ...

F and F0 are always the same, but the problematic part is F <-- G <-- F2 because the image of G in F (==F0) might have its generators sorted differently or altogether changed compared to the image of F1 in F0. Then the homology is nonzero at G. This is now fixed by instead returning

0 <-- M <-- F <-- F1 <-- F2 <-- ...

This is now always a correct free resolution (because we basically just take the one Singular gives us, F = F0 is always guaranteed). I hope this helps.

@wdecker
Copy link
Collaborator

wdecker commented Sep 12, 2023

@RafaelDavidMohr Thx for doing this.

I have a small remark: The methods

free_resolution(I::MPolyIdeal)

free_resolution(Q::MPolyQuoRing)

and also the "dummy" method

free_resolution(F::FreeMod)

should be adjusted to the syntax we have now for free_resolution.

@jankoboehm
Copy link
Contributor

As I understand it, we switch the presentation matrix originally computed by OSCAR to the (in case of mres, not fres) possibly modified one returned by Singular. Are we sure that this is the same module and not just an isomorphic one (then we have to keep track of the isomorphism), what if we resolve a submodule. We cannot change the generators, since the representation of the elements is referencing them. It also looks to me as if the grading is removed? Best have a quick talk in person once possible.

@RafaelDavidMohr
Copy link
Contributor Author

RafaelDavidMohr commented Sep 13, 2023

As I understand it, we switch the presentation matrix originally computed by OSCAR to the (in case of mres, not fres) possibly modified one returned by Singular. Are we sure that this is the same module and not just an isomorphic one (then we have to keep track of the isomorphism), what if we resolve a submodule. We cannot change the generators, since the representation of the elements is referencing them. It also looks to me as if the grading is removed? Best have a quick talk in person once possible.

Yes, in person (Saarbrücken for instance) is probably easier. I will however have a look at the grading issue and at the things @wdecker mentioned.

Edit: Actually, @jankoboehm, I am not sure what you mean by "the grading is removed". I tried a random example and I'm getting a graded free resolution. Could you specify?

@RafaelDavidMohr
Copy link
Contributor Author

@RafaelDavidMohr Thx for doing this.

I have a small remark: The methods

free_resolution(I::MPolyIdeal)

free_resolution(Q::MPolyQuoRing)

and also the "dummy" method

free_resolution(F::FreeMod)

should be adjusted to the syntax we have now for free_resolution.

See 347349c. I adjusted the docu accordingly, the docu for free_resolution(F::FreeMod) is not ideal, let me know if I should change it.

@wdecker
Copy link
Collaborator

wdecker commented Sep 18, 2023

This is good for now. However, for the mres part, we also need to minimize the presentation we work with. This requires some work in Singular first. We will fix this asap.

@wdecker wdecker enabled auto-merge (squash) September 18, 2023 08:13
@wdecker wdecker closed this Sep 18, 2023
auto-merge was automatically disabled September 18, 2023 08:16

Pull request was closed

@wdecker wdecker reopened this Sep 18, 2023
@wdecker wdecker merged commit b0569e7 into oscar-system:master Sep 18, 2023
23 of 28 checks passed
fieker pushed a commit that referenced this pull request Sep 29, 2023
* add `mres` with fix

* fixes no relation edge case

* adds `nres`, updated doc, adds tests

* fix typo `mres` -> `nres`

* compatibility changes to other `free_resolution` methods

---------

Co-authored-by: Rafael Mohr <Rafael.Mohr@lip6.fr>
Co-authored-by: ederc <ederc@mathematik.uni-kl.de>
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

6 participants