Skip to content

Added the Subring package - #3736

Open
fragandi wants to merge 35 commits into
Macaulay2:developmentfrom
fragandi:master
Open

Added the Subring package#3736
fragandi wants to merge 35 commits into
Macaulay2:developmentfrom
fragandi:master

Conversation

@fragandi

Copy link
Copy Markdown
Contributor

Hi everyone, here is a small package with methods for handling subrings based on the ones available in SAGBI and in InvariantRing

@d-torrance
d-torrance changed the base branch from master to development April 17, 2025 21:58

@d-torrance d-torrance left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the contribution!

In addition to the other comments, there's also a typo:

M2/Macaulay2/packages/Subring/SubringDoc.m2:282: accessable ==> accessible

Comment thread M2/Macaulay2/packages/Subring/Subring.m2 Outdated
Comment thread M2/Macaulay2/packages/Subring/Subring.m2 Outdated
@d-torrance

Copy link
Copy Markdown
Member

Also, "Subring" should be added to the =distributed-packages file.

Comment thread M2/Macaulay2/packages/Subring/Subring.m2
@fragandi

Copy link
Copy Markdown
Contributor Author

Also, "Subring" should be added to the =distributed-packages file.

I could not find this file, can you give me more details on the path?

@d-torrance

Copy link
Copy Markdown
Member

It's at M2/Macaulay2/packages/=distributed-packages

@fragandi

Copy link
Copy Markdown
Contributor Author

Also, "Subring" should be added to the =distributed-packages file.

I could not find this file, can you give me more details on the path?

NWM I thought a file could not start with = but I guess that's ok!

Comment thread .vscode/settings.json Outdated
Comment thread M2/Macaulay2/packages/Subring/SubringDoc.m2 Outdated
Comment thread M2/Macaulay2/packages/Subring/SubringDoc.m2
@mahrud

mahrud commented May 25, 2025

Copy link
Copy Markdown
Member

@fragandi is the plan to eventually make SubalgebraBases depend on this package? Because currently the subring conflict makes them not usable at the same time.

@fragandi

fragandi commented Jun 2, 2025

Copy link
Copy Markdown
Contributor Author

I have talked to Ollie @ollieclarke8787 before this PR and the idea is indeed to have Subrings as part of the M2 packages and then have SubalgebrasBases call on it. Maybe Ollie you can look into this conflict? Or let me know if I messed up something in the PR.

@mahrud

mahrud commented Jun 2, 2025

Copy link
Copy Markdown
Member

The current failure was because you forgot AuxiliaryFiles => true in the package header. The packages are fine alone, just not usable at the same time right now.

@d-torrance

Copy link
Copy Markdown
Member

@fragandi - Just a friendly reminder about this PR

@fragandi
fragandi marked this pull request as draft June 12, 2026 02:44
@fragandi

Copy link
Copy Markdown
Contributor Author

@ollieclarke8787 and I will follow up on this around the time of the Warwick workshop (6/29/26). Converted to draft until then.

@fragandi

Copy link
Copy Markdown
Contributor Author

@galettof and @trevorkarn might also be interested in this PR. We have a check-in meeting scheduled for June 22, let me know if you would like a link!

@ollieclarke8787

Copy link
Copy Markdown
Contributor

Some updates on the above changes:

  • The type Subring in the Subrings package is now completely interchangeable with the type Subring in SubalgebraBases so it should be a matter of importing the Subrings in SubalgebraBases and removing parts of that package.
  • I currently get an error when installing the package, which suggests there may be a problem with the documentation (since needsPackage "Subrings" seems to be fine) - but I'm having trouble tracking this down.

Comment thread M2/Macaulay2/packages/Subrings.m2 Outdated
Co-authored-by: Doug Torrance <douglas.a.torrance@gmail.com>
Comment on lines +73 to +74
flattenedRing = method()
flattenedRing Subring := S -> S#"flattenedRing"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not just install a method flattenRing Subring?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

flattenedRing was defined in SubalgebraBases so I pulled it across to Subrings, it should be possible to swap over to using flattenRing but the underlying meaning seems a little different: the flattenedRing S of S:Subring is equivalent to flattenRing ambient S. For now I've added flattenRing in b4777fe

@ollieclarke8787

Copy link
Copy Markdown
Contributor

isSubringElement now uses hooks, so it should be possible modify SubalgebraBases to build on top of it. TODO: add a new hook to a function that checks if a sagbi basis (of high enough degree) is already computed and return whether the element subducts to zero against the sagbi basis.

@ollieclarke8787

Copy link
Copy Markdown
Contributor

SubalgebraBases seems to be functioning with the Subrings package but the docs have a lot of ambiguous reference warnings.

@fragandi
fragandi requested a review from d-torrance August 12, 2026 13:28
@fragandi
fragandi marked this pull request as ready for review August 12, 2026 13:28
@fragandi

Copy link
Copy Markdown
Contributor Author

Spoke with Ollie @ollieclarke8787 today, his work addressed the comments above, let us know if there any other further suggested edits.

Comment thread M2/Macaulay2/packages/Subrings.m2 Outdated
Co-authored-by: Doug Torrance <douglas.a.torrance@gmail.com>
Comment thread M2/Macaulay2/packages/Subrings.m2 Outdated
remove `a package for` phrase from headline
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants