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 docu for matroid realization spaces as affine schemes #3982

Conversation

HechtiDerLachs
Copy link
Collaborator

As it turned out during the ICMS, these features were apparently not known to (some of?) the developers of matroid realization spaces. So I thought it might be worthwhile to add some documentation.

Still looking forward to testing and improving our smoothness tests for more complicated matroids!

ping: @bschroter , @dcorey2814 , @danteluber .

@LukasKuehne
Copy link
Contributor

Hi,
I have a question on this PR: does that mean that a matroid realization space is now automatically returned as an affine scheme? Does this has any performance implications? The realization space itself is quite costly sometimes and we therefore wanted to make the computation modular, i.e. for instance not automatically compute the saturation as this is too slow in many instances but only do it if a certain flag is true.
Best,
Lukas

Copy link

codecov bot commented Jul 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.33%. Comparing base (bfcfee3) to head (551bd13).
Report is 37 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3982      +/-   ##
==========================================
+ Coverage   84.30%   84.33%   +0.02%     
==========================================
  Files         596      596              
  Lines       81894    81895       +1     
==========================================
+ Hits        69041    69065      +24     
+ Misses      12853    12830      -23     
Files Coverage Δ
...ometry/Schemes/AffineSchemes/Objects/Properties.jl 90.49% <100.00%> (+0.04%) ⬆️

... and 1 file with indirect coverage changes

@HechtiDerLachs
Copy link
Collaborator Author

does that mean that a matroid realization space is now automatically returned as an affine scheme?

Yes. But that's the case since #1658. This PR does not change anything about that. It only documents these features.

Does this has any performance implications?

Not that I know of. Feel free to try it out!

The realization space itself is quite costly sometimes and we therefore wanted to make the computation modular,

I do not quite understand. If I call realization_space(my_matroid), then I get an object which is a MatroidRealizationSpace. It is as costly as calling your constructor and I did not interfere with any of your code there. If you plan to make any further changes or improvements on your realization spaces, this should also not be a problem.

In any case: Being an instance of an affine scheme should not affect any performance whatsoever. It just means that

  • the type MatroidRealizationSpace is not hanging below Any, but below AbsAffineScheme
  • that a function underlying_scheme(::MatroidRealizationSpace) has been implemented
  • that the result of the latter function is cached in an additional field of the MatroidRealizationSpace which is left empty by default.

Nothing more, nothing less.

@fieker fieker merged commit 1b48937 into oscar-system:master Aug 28, 2024
28 checks passed
@joschmitt joschmitt removed the triage label Aug 28, 2024
HechtiDerLachs added a commit to HechtiDerLachs/Oscar.jl that referenced this pull request Sep 13, 2024
…em#3982)

* Add section on matroid realization spaces as affine schemes.

* Fix bug.

* Update experimental/MatroidRealizationSpaces/docs/src/introduction.md

Co-authored-by: Lars Göttgens <[email protected]>

---------

Co-authored-by: Lars Göttgens <[email protected]>
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.

5 participants