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

Fixes mres and nres in free_resolution #3700

Merged
merged 9 commits into from
May 22, 2024

Conversation

RafaelDavidMohr
Copy link
Contributor

This PR will fix the mres and nres options in free_resolution using prune_with_map. Previously, using these options
might not return a minimal free resolution.

For using prune_with_map conveniently in free_resolution, I would like add a minimal keyword to presentation, if this is set to true we would return a minimal presentation, computed from the output of prune_with_map.

@wdecker Do we want a user-facing option to compute a minimal presentation? If not, then I will do this just internally.

@wdecker
Copy link
Collaborator

wdecker commented May 7, 2024

@RafaelDavidMohr As far as I can see, the new functionality is only used with mres. If everybody agrees, we do not need the proposed keyword. Or?

@wdecker
Copy link
Collaborator

wdecker commented May 7, 2024

@jankoboehm userfacing function named minimal_presentation?

@jankoboehm
Copy link
Contributor

jankoboehm commented May 8, 2024

Yes we want the keyword. There are use-cases for both options.

proj_map = hom(F0, SQ_new, gens(SQ_new), check=false)
F0_to_SQ = compose(proj_map, phi)
F0_to_SQ.generators_map_to_generators = true
AbstractAlgebra.set_name!(F0, "$br_name^$(ngens(SQ.sub))")
Copy link
Member

Choose a reason for hiding this comment

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

Why do you ned to set this name here and below? Since F0 and F1 are free module anyway, couldn't this be done generally as part of the show methods for free modules?

As it is, this hardcodes the name of the basering, so if it changes later on, it will look ugly. Likewise if there is no name set, you hardcode br forever.

Copy link
Member

Choose a reason for hiding this comment

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

This is the current standard for presentations/resolutions/etc. and the same at other places. Thus I would keep it like this here right now and would instead think of some way to replace all of these instances by some AA printing magic in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah if this is pervasive we should not try to deal with it in this PR. But perhaps we can open an issue as a reminder?

Or perhaps we just add it to #2891

Copy link
Member

Choose a reason for hiding this comment

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

See #3730

@RafaelDavidMohr
Copy link
Contributor Author

@wdecker @jankoboehm do you have an example where presentation produces something non-minimal that I can use as a test for the minimal free resolution?

Copy link

codecov bot commented May 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.36%. Comparing base (357c46b) to head (f2a684b).
Report is 15 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3700      +/-   ##
==========================================
+ Coverage   81.34%   81.36%   +0.01%     
==========================================
  Files         577      577              
  Lines       78618    78674      +56     
==========================================
+ Hits        63952    64011      +59     
+ Misses      14666    14663       -3     
Files Coverage Δ
src/Modules/UngradedModules/FreeResolutions.jl 89.53% <100.00%> (ø)
src/Modules/UngradedModules/Presentation.jl 69.54% <100.00%> (+5.04%) ⬆️

... and 73 files with indirect coverage changes

@wdecker
Copy link
Collaborator

wdecker commented May 10, 2024 via email

@wdecker
Copy link
Collaborator

wdecker commented May 10, 2024 via email

@RafaelDavidMohr
Copy link
Contributor Author

one more: S, (x0, x1, x2, x3, x4) = graded_polynomial_ring(GF(3), ["x0", "x1", "x2", "x3", "x4"]); m = ideal(S, [x1^2+(-x1+x2+x3-x4)x0, x1x2+(x1-x3+x4)x0, x1x3+(-x1+x4+x0)x0, x1x4+(-x1+x3+x4-x0)*x0, x2^2+(x1-x2-x4-x0)x0, x2x3+(x1-x2+x3+x4-x0)x0, x2x4+(x1+x2-x3-x4-x0)*x0, x3^2+(x3+x4-x0)x0,x3x4+(-x3-x4+x0)*x0, x4^2+(x1+x3-x4-x0)*x0]); Qm, _ = quo(S, m); FQm = free_resolution(Qm) M, _ = image(map(FQm, 1))

Do we know the correct ranks for the minimal free resolution in this example so I can test?

@wdecker
Copy link
Collaborator

wdecker commented May 13, 2024

In _presentation_minimal please catch border case of a free module before entering
AbstractAlgebra.set_name!(F0, "$br_name^$(ngens(SQ.sub))")

@wdecker
Copy link
Collaborator

wdecker commented May 14, 2024

There is a problem with the print function:

julia> R, (x,y,z) = polynomial_ring(QQ, ["x", "y", "z"]);

julia> A = R[x 1 y; 2x^2 z^2 3y^2];

julia> M = cokernel(A);

julia> PM = presentation(M)
0 <---- M <---- R^3 <---- R^2

julia> MPM = _presentation_minimal(M)
0 <---- M <---- R^3 <---- R^1

julia> matrix(map(MPM,1))
[-2x^2 + xz^2 -3y^2 + yz^2]

julia> MPM[0]
Free module of rank 2 over R

@RafaelDavidMohr
Copy link
Contributor Author

In _presentation_minimal please catch border case of a free module before entering AbstractAlgebra.set_name!(F0, "$br_name^$(ngens(SQ.sub))")

This case is caught in presentation already, and the user will only access _presentation_minimal via the minimal kw of presentation. Should I nonetheless treat this case seperately?

@RafaelDavidMohr
Copy link
Contributor Author

M = cokernel(A);

Fixed with f2a684b

@wdecker
Copy link
Collaborator

wdecker commented May 14, 2024 via email

@fingolfin
Copy link
Member

@wdecker I am afraid I don't understand what you are asking about. What is "this internal function", and which border case?

Happy to talk about it during triage today if anyone would like to.

@lgoettgens
Copy link
Member

@wdecker I am afraid I don't understand what you are asking about. What is "this internal function", and which border case?

Happy to talk about it during triage today if anyone would like to.

I think I understood the issue and can try to explain it later in triage if wdecker is not there

@RafaelDavidMohr
Copy link
Contributor Author

I added the discussed minimization of the kernel in _presentation_minimal. I added it with an optional keyword, so that it can easily be integrated into free_resolution.

Currently this uses prune_with_map. I tried to use Singular.mres but this function seems to ignore the length argument.

@wdecker Should I make a Singular issue regarding this?

@ederc
Copy link
Member

ederc commented May 16, 2024

Can you give a concrete example where Singular.mres ignores the length argument? If I compute a free resolution with current Oscar master using algorithm = :mres and length set, it seems to compute up to the correct length.

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.

This is ready to go as soon as the tests are green. Please check tests.

@RafaelDavidMohr
Copy link
Contributor Author

Can you give a concrete example where Singular.mres ignores the length argument? If I compute a free resolution with current Oscar master using algorithm = :mres and length set, it seems to compute up to the correct length.

Maybe I'm misunderstanding the concept of length of a free resolution, but for example

julia> R, (x, y, z) = polynomial_ring(GF(65521), ["x", "y", "z"]);

julia> I = ideal(R, [x^2, x*y, y]);

julia> M = ideal_as_module(I);

julia> sf = Oscar.singular_module(ambient_free_module(M));

julia> sm = Singular.Module(base_ring(sf), [sf(repres(g)) for g in gens(M)]...);

julia> Singular.mres(sm, 1)
Singular resolution: R^1 <- R^2 <- R^1

julia> Singular.mres(sm, 2)
Singular resolution: R^1 <- R^2 <- R^1

julia> Singular.mres(sm, 3)
Singular resolution: R^1 <- R^2 <- R^1

@wdecker
Copy link
Collaborator

wdecker commented May 21, 2024

@RafaelDavidMohr In your example, the resolution is short, so entering an increased length will not change anything:

julia> R, (x, y) = polynomial_ring(GF(65521), ["x", "y"]);

julia> I = ideal(R, [x^2, x*y, y]);

julia> M = ideal_as_module(I);

julia> FM = free_resolution(M, algorithm = :mres, length = 1)
Free resolution of M
R^3 <---- R^2
0 1

julia> FM = free_resolution(M, algorithm = :mres, length = 0)
Free resolution of M
R^3 <---- R^2 <---- 0
0 1 2

So why now does Singular print this:

julia> Singular.mres(sm, 1)
Singular resolution: R^1 <- R^2 <- R^1

I guess, this is due to the fact that you use mres. So you have to go one step further in order to minimalize. @hannes14 ?

@ederc ederc marked this pull request as ready for review May 22, 2024 10:21
@ederc ederc merged commit d9d6bae into oscar-system:master May 22, 2024
26 checks passed
wdecker added a commit that referenced this pull request May 24, 2024
jankoboehm pushed a commit that referenced this pull request May 29, 2024
* Adjusting docu following PR #3700

* More illustrative example

* Documenting more methods and improving text

* remove white space
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.

6 participants