Skip to content

Adding documentation and cleaning up arings#4441

Draft
MichaelABurr wants to merge 12 commits into
Macaulay2:developmentfrom
MichaelABurr:arings
Draft

Adding documentation and cleaning up arings#4441
MichaelABurr wants to merge 12 commits into
Macaulay2:developmentfrom
MichaelABurr:arings

Conversation

@MichaelABurr

Copy link
Copy Markdown
Member

Work from the C/C++ refactoring group. This pull request will

  1. Add additional tests for aring types in the unit-tests directory.
  2. Add documentation to the individual aring-*.hpp files
  3. Add documentation for both humans and AIs that describes how to create an aring
  4. Generally clean up the directories, checking if TODOs have been completed and simplifying code.

d-torrance and others added 9 commits June 12, 2026 15:32
Ring types that cannot be set from a BigReal no longer need to define
a boilerplate set_from_BigReal returning false; get_from_BigReal uses
SFINAE to detect whether the member exists and falls back to false
otherwise.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the hand-written per-ring get_from_double overloads with a
single template that uses SFINAE to detect whether a ring defines
set_from_double, falling back to false otherwise. As a side effect,
ARingQQGMP's existing set_from_double (previously only used by
mylift) is now also used for from_double, giving an exact
double -> QQ conversion.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the hand-written per-ring get_from_BigComplex overloads with
a single template that uses SFINAE to detect whether a ring defines
set_from_BigComplex, falling back to false otherwise.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the hand-written per-ring get_from_complex_double overloads
with a single template that uses SFINAE to detect whether a ring
defines set_from_complex_double, falling back to false otherwise.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the hand-written per-ring get_from_Interval overloads with a
single template that uses SFINAE to detect whether a ring defines
set_from_Interval, falling back to false otherwise.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the hand-written ARingCCi overload of get_from_ComplexInterval
with a single template that uses SFINAE to detect whether a ring
defines set_from_ComplexInterval, falling back to false otherwise.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The three overloads of mylift with second argument ARingRRi (for
ARingRR, ARingRRR, and ARingQQ) had identical bodies, differing only
in the type of the first ring. Replace them with one function
template parameterized on the first ring type.
Use some templates in aring-translate.hpp
@d-torrance

d-torrance commented Jun 13, 2026

Copy link
Copy Markdown
Member

A couple tests were failing because one of the changes from my PR made it so that ring maps from RR to QQ now use the lifting code instead of failing.

These tests ensured that applying these ring maps raised errors, and according to the comments that pointed to #473 , the main point of adding the tests was to show that they no longer segfaulted.

I'm proposing that instead of just failing, they should actually work, e.g., sub(matrix 1.0, QQ) should give us matrix(QQ, 1). So I updated these tests accordingly.

@pzinn

pzinn commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

there are no ring maps from RR to QQ -- I'm confused

@d-torrance

Copy link
Copy Markdown
Member

Well, we can lift from RR to QQ. And that's what ring maps currently use when we have large precision (i.e., when we're using mpfr instead of doubles). So currently, before this PR, we have:

i1 : f = map(QQ, RR_53)

o1 = map (QQ, RR  , {})
                53

o1 : RingMap QQ <-- RR
                      53

i2 : f 1.0
stdio:2:1:(3):[1]: error: cannot map double to ring type

i3 : g = map(QQ, RR_100)

o3 = map (QQ, RR   , {})
                100

o3 : RingMap QQ <-- RR
                      100

i4 : g 1.0

o4 = 1

o4 : QQ

This PR is proposing that f should behave like g.

@pzinn

pzinn commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

lifting is not a ring map, that's why we use it when... there's no ring map...
when was this RR_100 behaviour introduced?

@d-torrance

Copy link
Copy Markdown
Member

Good question ... it might have been pretty recently. My guess is maybe #4280, which moved the lifting code to the engine.

@pzinn

pzinn commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

I think I understand the point, this is RR_53 or RR_100, not RR.

@d-torrance d-torrance added the Engine Macaulay2/e label Jun 13, 2026
@d-torrance

Copy link
Copy Markdown
Member

Another example showing that map isn't necessarily a homomorphism but lifts if possible:

i1 : f = map(ZZ, QQ);

o1 : RingMap ZZ <-- QQ

i2 : f (5/1)

o2 = 5

d-torrance and others added 2 commits June 15, 2026 20:15
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@pzinn

pzinn commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

well, that one is 100% wrong. What's ambiguous about the name of the class "RingMap"?

@MichaelABurr

Copy link
Copy Markdown
Member Author

I'm torn on this one a bit. I agree with Paul that there should be a clear specification that explains when map, lift, promote, and substitute should all be used. Of these, substitute is perhaps the one that can violate the most rules, as it is intended to guess the right answer.

I think that, with a specification in place, it would be good to go through these back-end maps, lifts, and promotes to see which ones violate the specification. However, I do have a worry that deleting too many of these back-end functions will have unintended consequences (in that these maps were used as a convenient place to store information so that function calls were consistent).

I'm thinking that it might make sense to look through these back-end maps, lifts, and promotes in a separate pull request to try to understand the consequences of the changes that Paul is suggesting. Although, I could be convinced otherwise.

@Devlin-Mallory

Copy link
Copy Markdown
Contributor

Regarding the use of (not well-defined) ring maps for sub, see #4029 for a potentially related issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Engine Macaulay2/e

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants