Make promote/lift and ring maps honest about canonical homomorphisms#62
Draft
d-torrance wants to merge 9 commits into
Draft
Make promote/lift and ring maps honest about canonical homomorphisms#62d-torrance wants to merge 9 commits into
d-torrance wants to merge 9 commits into
Conversation
ARingGFFlint and ARingGFFlintBig already expose dimension() directly; ARingGFM2 delegates to its internal GF object. Needed for the canonical-map predicate that checks GF(p^m) → GF(p^n) divisibility. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Encodes the canonical ring-map DAG (ZZ→QQ→RR→CC, ZZ→Fp→GF) as an enum and BFS-based reflexive-transitive closure. hasCanonicalMap combines structural precedence with characteristic matching and GF(p^m)→GF(p^n) dimension divisibility into a single authoritative predicate. gfDimension() accesses the concrete ARing type via static_cast keyed on ringID(), avoiding a new virtual function on Ring (which would change the vtable layout for all Ring subclasses). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Rejects structurally invalid conversions (e.g. QQ→ZZ, QQ→Fp) early via the new hasCanonicalMap predicate, before reaching the ring-pair switch. lift shares the same guard because it is the partial inverse of the same canonical map. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Tests structureOf (RingID→RingStructure mapping), structurePrecedes reflexivity, valid promote paths through the char-0 tower and finite-field family, and invalid maps (QQ↛ZZ, QQ↛Fp, RR↛QQ, etc.). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
In map(Ring, Ring, Matrix), when the while loop reaches a basic coefficient ring A (numgens A == 0), check isPromotable(A, R) before proceeding. This catches cases like map(ZZ, QQ) at construction time rather than silently producing an invalid map. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This method tried to promote rational numbers element-wise (numerator divided by denominator) to arbitrary rings, succeeding whenever the denominator happened to be a unit. This made promote(2/1, ZZ[x]) work even though there is no ring homomorphism QQ -> ZZ[x], and caused isPromotable(QQ, ZZ[x]) to return true. Per-ring initialization in commonEngineRingInitializations already registers promote(QQ, R) for every ring R that QQ genuinely embeds into (QQ[x], RR, CC, etc. via their baseRings). Removing the global fallback makes isPromotable honest. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replaces the ~130-line nested switch in ConcreteRing::promote and the ~100-line nested switch in ConcreteRing::lift with a RingVariant + toVariant helper and a two-argument std::visit lambda that calls mypromote/mylift directly on the unwrapped concrete ring references. The old lift switch had three latent bugs now fixed automatically: - ring_RRi and ring_CCi source arms were missing (those mylift specializations in aring-translate.hpp were unreachable) - The ring_CC→ring_CCC and ring_CCC→ring_CC lift template args were swapped Structural drift is now impossible: adding a new ring type requires only adding it to RingVariant and toVariant; the compiler enforces coverage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Collaborator
Author
|
Hmm, this breaks the It's easy to fix the documentation node -- just build the matrix from scratch over |
Collaborator
Author
|
Or maybe rather than calling |
promote(RR, R[x]) is installed using class(0_(RR_53)) = RR as the key, so isPromotable(RR_53, R[x]) must look up by the ring family, not the specific precision ring. Fixes map(R[x], R) for RR, CC, RRi, CCi and their variants. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Install promote(S, RingElement) := basicPromote inside the ZZp constructor so that element-level dispatch finds it (elements dispatch on class(x) = the ring, while Ring-typed methods dispatch on class(ring) = QuotientRing). Add isPromotable(QuotientRing, Ring) outside the constructor: for finite prime fields it checks char equality; for other QuotientRings it falls back to lookup(promote, R, S). This fixes the dispatch mismatch that caused isPromotable(ZZp 101, ZZp(101, Strategy => "Aring")) to return false. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Claude and I worked on some pretty big refactoring after our discussions on Friday. Claude-written summary below.
Draft for now to test the builds.
Edit: The
set_from_*->setcommits were split off into #63.Summary
This branch makes Macaulay2's
promote,lift, andmap(Ring, Ring, ...)machinery agree with the actual mathematics: a conversion between two basic
rings is only allowed when a canonical ring homomorphism exists. Previously
several structurally invalid conversions silently succeeded (e.g.
promote(2/1, ZZ[x])ormap(ZZ, QQ)), andisPromotablereportedtruefor maps that don't exist.
Along the way it cleans up the engine-side ARing conversion API
(
set_from_*→set,get_from_*→try_set), replaces two large hand-written promote/lift switch statements with compiler-checked
std::variantdispatch (fixing three latent
liftbugs in the process), and fixes a set ofregressions over the
ARingGFM2Galois-field backend that the rename exposed.Motivation
The canonical ring-map DAG for Macaulay2's basic rings is
Nothing enforced this.
isPromotableandpromotewould accept conversionsthat run against the arrows (
QQ → ZZ,QQ → Fp,RR → QQ), and a globalpromote(QQ, RingElement)fallback would promote a rational element-wise intoany ring whenever the denominator happened to be a unit there. As a result,
ring maps could be constructed that have no underlying homomorphism, and
predicates that callers rely on returned wrong answers.
Key changes
Canonical-map predicate (engine)
basic-rings/ring-structure.{hpp,cpp}introduces aRingStructureenum that collapses precision and backend (RR/RRR →
RealField, all ZZpbackends →
PrimeField, etc.) and astructurePrecedespartial ordercomputed as the reflexive-transitive closure of the canonical-map DAG.
hasCanonicalMap(R, S)is the single authoritative predicate: it combinesthe structural order with characteristic matching for finite-field families
and
GF(p^m) → GF(p^n)dimension divisibility (m | n).gfDimensionreaches the concrete ARing viastatic_castkeyed onringID(), deliberately avoiding a new virtual onRing(which wouldchange the vtable layout for every
Ringsubclass).ARingGFM2gains adimension()accessor to matchARingGFFlint{,Big}.ConcreteRing::promote/liftnow reject structurally invalid conversionsearly via
hasCanonicalMap, before reaching the ring-pair switch.promote/lift/isPromotablehonesty (interpreter)promote(QQ, RingElement)element-wise fallback. Theper-ring
promote(QQ, R)registrations incommonEngineRingInitializationsalready cover every ring
QQgenuinely embeds into, soisPromotableisnow honest.
map(Ring, Ring, Matrix)now errors at construction time when it reaches abasic coefficient ring
Awith no canonical mapA → R, instead ofsilently producing an invalid map.
isPromotable(InexactField, Ring)looks up by ring family (class 0_R)rather than the specific precision ring, fixing
map(R[x], R)forRR/CC/RRi/CCi.isPromotable(QuotientRing, Ring)and an in-constructorpromote(S, RingElement) := basicPromotefix the dispatch mismatch forcross-backend
ZZprings (elements dispatch on the ring,Ring-typedmethods on
QuotientRing).ARing conversion API cleanup (engine)
set_from_long,set_from_mpz,set_from_mpq,set_from_double,set_from_doubles,set_from_BigReal(s),set_from_BigComplex,set_from_Interval,set_from_ComplexInterval, andset_from_intare allrenamed to overloads of
set()— the source type is already encoded in theargument. Duplicate
set_from_complex_double/set_from_complex_mpfrmethods are removed. Added
set(ElementType&, int)overloads wherelongand
doubleoverloads would otherwise make anintargument ambiguous.get_from_*are renamed totry_set(theyattempt
R.set(elem, val)and report whether the ring supports it). Thehas_set_from_*concept detectors keep theirfrom_infix.ConcreteRing::promote/liftreplace a ~130-line and a ~100-line nestedswitch with a
RingVariant+toVarianthelper and two-argumentstd::visit. Adding a new ring type now requires only extendingRingVariant/toVariant; the compiler enforces coverage. This refactorfixed three latent
liftbugs automatically: missingring_RRi/ring_CCisource arms, and swapped
CC ↔ CCCtemplate arguments.ARingGFM2regression fixesARingGFM2stores elements as table indices, not integers, so after therename
ring().set(result, 1)resolved to the element-copy overload and wrotea raw index instead of converting
1through the GF table. Fix: dropset(elem&, elem)fromARingGFM2soset()always means "convert frominteger", and replace element-copy uses of
ring().set(...)withring().copy(...)across the mutable-matrix and ring infrastructure(
dmat-lu*,mat-elem-ops,mat-arith,mat-util,smat,aring-glue).This restores
mutableMatrix, polynomial coefficient extraction, anddetover
GF(2^n)rings built with the"New"ARing strategy.Tests
unit-tests/RingStructureTest.cppcoversstructureOf,structurePrecedesreflexivity, valid promote paths through both thechar-0 tower and the finite-field family, and invalid maps
(
QQ ↛ ZZ,QQ ↛ Fp,RR ↛ QQ, …).set()/try_setrename.Notes
This is purely an engine + interpreter change; no documentation rebuild is
required beyond removing the now-deleted
(promote, QQ, RingElement)from theundocumentedlist.