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 for the degree function #3298

Merged
merged 103 commits into from
Feb 21, 2024
Merged

Conversation

HechtiDerLachs
Copy link
Collaborator

This addresses #2999 .

The PR contains the improved changes from #3238, rebased on #3237.

It can be rebased again, once #3237 is merged.

For the last testset in test/Modules/ModulesGraded.jl I now get the following timings:

julia> @time Oscar._alt_simplify(M); # M is Omega without grading.
  0.490810 seconds (2.12 M allocations: 113.333 MiB)

julia> @time Oscar._alt_simplify(Omega);
  0.667269 seconds (3.38 M allocations: 151.342 MiB)

so the gradings still seem to make things slower. But computations now at least finish in comparable time.

@thofma
Copy link
Collaborator

thofma commented Feb 19, 2024

What is the plan for the :task keyword, which is still there for the truncate function?

thofma
thofma previously requested changes Feb 19, 2024
Copy link
Collaborator

@thofma thofma left a comment

Choose a reason for hiding this comment

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

Just adding this here, so that we don't accidentally merge this without resolving the :task question.

@HechtiDerLachs
Copy link
Collaborator Author

HechtiDerLachs commented Feb 19, 2024

It turns out, this PR has another problem (apart from the tests failing again):

Edit: This seems to be about the flattenings of modules over rather exotic rings. These can not be expected
to work in general and so I think it's safe to leave this cleanup to another PR. That this did not blow up on master is only luck. So we should have the problems below in mind, but not have this PR blocked by that.

  kk = GF(40009);
  B, (t,) = polynomial_ring(kk, [:t]);
  IP5 = projective_space(B, [:u, :v, :w, :x, :y, :z]);
  S = homogeneous_coordinate_ring(IP5);
  (u, v, w, x, y, z) = gens(S);
  f = x*y - t^2*v^2 - w^2;
  g = z*w - t*u*v;
  I = ideal(S, [f, g]);
  X, inc_X = sub(IP5, I);
  Om1X = Oscar.relative_cotangent_module(X);
  SX = homogeneous_coordinate_ring(X);
  F1X = graded_free_module(SX,[0]);
  TX,_ = hom(Om1X,F1X);
  TXamb,_ = pushforward(inc_X,TX);
  W1X,_ = hom(TX,F1X)
  M, a, b = Oscar._alt_simplify(W1X)
  is_isomorphism(a) # true
  is_isomorphism(b) #false
  compose(a, b) == identity_map(domain(a)) # true
  compose(b, a) == identity_map(domain(b)) # true

If one proceeds by

K, inc = kernel(b)
v = inc(K[1])
is_zero(v) #false 
w = b(v)
is_zero(w) # true
is_zero(a(w)) # false 

The only thing I found so far is that the SubModuleOfFreeModule layer of the modules is still not sufficiently generic so that in one way or the other Groebner basis reduction via singular is called on modules over towers of polynomial rings. But this will be nasty to debug, I guess. Interestingly, the above code does not fail on the latest master.

@jankoboehm
Copy link
Contributor

Just adding this here, so that we don't accidentally merge this without resolving the :task question.

The idea is to do that issue in a separate PR. This one should limit to the degree check question.

@jankoboehm
Copy link
Contributor

Interestingly, the above code does not fail on the latest master.
Before merging, we should find out why it works on master, and now not any more with this modification. Otherwise one surely will be able to produce other examples which fail. Perhaps I have an idea.

Reduction is part of the layer.

@HechtiDerLachs
Copy link
Collaborator Author

Before merging, we should find out why it works on master, and now not any more with this modification. Otherwise one surely will be able to produce other examples which fail. Perhaps I have an idea.

@jankoboehm : This is not a citation, but what you said, correct?

In any case, I have a fix now and I understand why it worked on master but not here. This can be pushed to #3348 . But it will be several hundred lines of code unrelated with this PR's original intention. So I think, it should also go separately.

@HechtiDerLachs
Copy link
Collaborator Author

I had no choice but to also include the changes of the flattenings of modules. Otherwise the tests wouldn't run again. Also, they were running for the wrong reasons before, so I think, it's better this way.

@HechtiDerLachs
Copy link
Collaborator Author

Good to be merged. Only non-required tests have not run yet and I don't see how to trigger them.

@jankoboehm jankoboehm merged commit cfb3413 into oscar-system:master Feb 21, 2024
20 checks passed
@lgoettgens

This comment was marked as outdated.

@jankoboehm jankoboehm added the backport 1.0.x Should be backported to the release 1.0 branch label Feb 22, 2024
@lgoettgens
Copy link
Member

@benlorenz @aaruni96 some heads-up for the backport: This PR introduced some code that is not compatible with AbstractAlgebra v0.40.0. For the master branch, this was replaced by the new interface in #3374. For the backporting of these two commits, you probably either need to keep the order as on the master branch (first cherry-pick this and #3374 afterwards), or apply cb62fb3 (#3374) and then 404ac73 (#3374) after cherry-picking this PR to make it work again.

@benlorenz
Copy link
Member

Thanks for the info, I did backport #3374 earlier today with some small conflicts that I needed to resolve. (due to not having this PR yet)
But I will now try to redo this in the correct order just to be safe.

benlorenz pushed a commit that referenced this pull request Feb 22, 2024
* Remove :task from sub methods for modules.

* Introduce new function submodule.

* Remove all deprecated uses of sub.

* Export submodule.

* Fix tests.

* Fix tests again.

* Register morphisms in general.

* Rebase network of natural maps on WeakKeyIdDicts.

* Clean up the morphisms network and dont store the actual morphisms.

* Some small fix for Wolfram.

* Some fixes and marking the test as broken again.

* Fix documentation.

* Another small fix for Wolfram.

* Fix the fix.

* Fix method selection.

* Introduce kw argument for caching morphisms.

* Redirect previous usages.

* Adjust tests.

* submodule -> sub_object.

* Fix docstrings and signatures for quo.

* Fix methods for quo.

* Adjust documentation.

* Fix up usages of quo.

* Export new functions.

* Some fixes.

* Fix tests.

* Fix tests.

* Fix tests.

* Fix tests.

* Fix tests.

* Fix tests.

* Squashed changes.

* Fix tests.

* Fix up the truncation.

* New rebase.

* Fix tests.

* WIP with debug messages.

* Progress in debugging.

* WIP on debugging.

* Write truely generic presentation and kernel methods [no ci].

* Repair graded_map to accept zero entries.

* Add tensor decomposition function to return value.

* Add some assertions.

* Repair kernel routine.

* Rewrite hom methods for modules.

* Add generic method for simplification of SubquoModuleElems.

* Add missing check argument.

* Fix or disable brittle tests.

* Fix faulty merge.

* Go back to old hom.

* Switch tests to old hom.

* Clean up some deprecated assertions.

* Repair truncate.

* Disable internal checks.

* Update docs/src/CommutativeAlgebra/ModulesOverMultivariateRings/free_modules.md

* Revert "Go back to old hom."

This reverts commit a6b7c15.

* Revert "Switch tests to old hom."

This reverts commit 178eaf3.

* Disable duplicate method.

* Adjust printing.

* Fix tests.

* Fix keyword argument for duals.

* Fix doctests.

* Fix tests.

* Fix doctests.

* Delete some falsely added files.

* Remove deprecated code.

* Another round of disabling internal checks.

* Disable various internal checks.

* Disable further internal checks.

* Set random seeds in tests.

* Adjust use of keyword argument.

* Fix more random seeds.

* Fix the generic simplify method for SubquoModuleElems.

* Revert "Remove deprecated code."

This reverts commit 190256b.

* Repair revert to old hom.

* Repair preimage function.

* Repair degree function.

* Adjust tests to use of repaired old hom.

* Add dummy simplify function for FreeModuleElems.

* Fix doctests.

* Repair simplify.

* Fix documentation.

* Fix doctests.

* Some tuning.

* Fix tests.

* Bugfix from running book code run.

* Remove some debugging artifacts.

* Fix faulty merge.

* Restrict signature.

* Update flattenings of modules over towers of polynomial rings to make the tests run again.

* Small fix.

* Disable the superfluous kernel routine.

* Restrict some further signatures to cases Singular can handle.

* Add tests for flattenings of modules.

* Readd methods which are not superfluous, yet.

---------

Co-authored-by: Lars Göttgens <[email protected]>
Co-authored-by: Lars Göttgens <[email protected]>
(cherry picked from commit cfb3413)
@benlorenz benlorenz mentioned this pull request Feb 22, 2024
33 tasks
@benlorenz benlorenz removed the backport 1.0.x Should be backported to the release 1.0 branch label Feb 22, 2024
benlorenz added a commit that referenced this pull request Feb 23, 2024
### Backported PRs

- [x] #3367
- [x] #3379 
- [x] #3369
- [x] #3291
- [x] #3325
- [x] #3350 
- [x] #3351
- [x] #3365 
- [x] #3366
- [x] #3382
- [x] #3373
- [x] #3341
- [x] #3346
- [x] #3381
- [x] #3385
- [x] #3387 
- [x] #3398 
- [x] #3399 
- [x] #3374 
- [x] #3406 
- [x] #2823
- [x] #3298
- [x] #3386 
- [x] #3412 
- [x] #3392 
- [x] #3415 
- [x] #3394
- [x] #3391
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

7 participants