feat(math): add integer multiplicative normal-form operations - #2038
feat(math): add integer multiplicative normal-form operations#2038morluto wants to merge 3 commits into
Conversation
…re, boundary, Kolmogorov quotient, and continuity check Add the finite_topology_spaces domain implementing exact, bounded, deterministic finite topological space operations over immutable finite topological spaces represented by specialization preorders: - topology.finite.interior.compute: largest open set contained in a subset - topology.finite.closure.compute: smallest closed set containing a subset via specialization preorder up-set - topology.finite.boundary.compute: closure minus interior - topology.finite.kolmogorov_quotient.compute: T0 quotient identifying points with the same minimal open neighbourhood - topology.finite.continuity_check.compute: continuity check via specialization preorder monotonicity (x' <= x implies f(x') <= f(x)) On a finite set, every topology is Alexandrov, so the topology is equivalently represented by its specialization preorder. The FiniteTopologicalSpace value parses only well-formed spaces with reflexive preorders and in-range indices. Closes #1920
…ests - Import TOOLS and ADMISSIONS in catalog/builtins.py. - Update frozen admission baselines and add the finite_topology_spaces schema-snapshot fragment (5 operations). - Add 14 focused tests (interior, closure, boundary, Kolmogorov quotient, continuity check, validation).
Add four exact operations for canonical integer multiplicative normalization: - integer.perfect_power.profile.compute: maximal perfect-power profile with structural zero/unit/nonunit classification, handling negative integers by restricting to odd maximal exponents. - integer.k_free_decomposition.compute: canonical n = a^k * c where a >= 1, c has the sign of n, and no prime to the k-th power divides |c|. - integer.squarefree_decomposition.compute: direct k=2 specialization with square factor, signed squarefree part, and exponent parity rows. - quadratic_radical.positive_integer.normalize.compute: canonical sqrt(n) = s * sqrt(d) with ZERO/RATIONAL/IRRATIONAL classification. All operations use SymPy factorint as a private backend and return Jacobian-owned typed values with exact reconstruction validators. Closes #1893
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| class PerfectPowerProfileRequest(StrictModel): | ||
| """One canonical integer for the maximal perfect-power profile.""" | ||
|
|
||
| value: str = Field(pattern=r"^-?(?:0|[1-9][0-9]*)$", max_length=_MAX_INTEGER_LENGTH) | ||
|
|
||
|
|
||
| class KFreeDecompositionRequest(StrictModel): | ||
| """One integer and exponent k >= 2 for k-free decomposition.""" | ||
|
|
||
| value: str = Field(pattern=r"^-?(?:0|[1-9][0-9]*)$", max_length=_MAX_INTEGER_LENGTH) | ||
| k: int = Field(ge=2, le=10_000) | ||
|
|
||
|
|
||
| class SquarefreeDecompositionRequest(StrictModel): | ||
| """One integer for squarefree decomposition.""" | ||
|
|
||
| value: str = Field(pattern=r"^-?(?:0|[1-9][0-9]*)$", max_length=_MAX_INTEGER_LENGTH) | ||
|
|
||
|
|
||
| class QuadraticRadicalNormalizeRequest(StrictModel): | ||
| """One nonnegative integer for positive quadratic-radical normalization.""" | ||
|
|
||
| value: str = Field(pattern=r"^(?:0|[1-9][0-9]*)$", max_length=_MAX_INTEGER_LENGTH) |
There was a problem hiding this comment.
🔴 New integer normalization tools accept 256-digit numbers they cannot actually factor, so requests can hang the server
The four new normalization tools accept integers with up to 256 decimal digits (max_length=_MAX_INTEGER_LENGTH at src/jacobian/math/number_theory/_normal_forms.py:32) even though each one must fully factor the number, so an ordinary accepted request can run essentially forever and block the process.
Impact: A single accepted request with a large hard-to-factor number never returns and ties up the server indefinitely.
Advertised input domain exceeds what the factorization backend can exhaust
All four kernels in src/jacobian/math/number_theory/_normal_form_operations.py call _factorint (src/jacobian/math/number_theory/_normal_form_operations.py:30-34), which runs SymPy factorint on |n|. Complete factorization of a general 256-digit integer (e.g. an RSA-style semiprime) is computationally infeasible, so the request contract is far broader than the implementation can establish.
The existing number-theory domain already recognises this: operations that factor their input use FactorizationInteger with max_length=_MAX_FACTORIZATION_LENGTH = 12 and an explicit comment that "These small bounds deliberately keep arithmetic functions that may factor their input ... safe for in-process SymPy execution" (src/jacobian/math/number_theory/_models.py:28-58). The new requests instead reuse the 256-digit bound intended for gcd/lcm-style arithmetic.
AGENTS.md ("Mathematical boundedness is a proof obligation") requires separately bounding accepted input and algorithmic work, and shrinking the request when the algorithm cannot exhaust it.
Affected request models: PerfectPowerProfileRequest, KFreeDecompositionRequest, SquarefreeDecompositionRequest, QuadraticRadicalNormalizeRequest (src/jacobian/math/number_theory/_normal_forms.py:29-51).
Prompt for agents
All four new normal-form requests in src/jacobian/math/number_theory/_normal_forms.py accept integers up to 256 decimal digits, but every kernel in _normal_form_operations.py performs a complete prime factorization via SymPy factorint. Complete factorization is infeasible for large general integers, so an accepted request can run unboundedly. The number-theory domain already has a conservative bound for factoring operations (FactorizationInteger, max_length 12, see src/jacobian/math/number_theory/_models.py) with an explicit comment about in-process SymPy safety. Consider reusing that bounded string type (or another explicitly justified, tested bound) for the four new requests, and add boundary tests at the accepted maximum.
Was this helpful? React with 👍 or 👎 to provide feedback.
| class SubsetRequest(StrictModel): | ||
| """Operate on a subset of points.""" | ||
|
|
||
| space: FiniteTopologicalSpace | ||
| subset: tuple[int, ...] = Field(default=()) |
There was a problem hiding this comment.
🔴 Asking for the closure of a subset with an invalid point number crashes instead of returning a result
The subset of points supplied to the interior/closure/boundary tools is never checked against the space before use, so an out-of-range point number is silently ignored by the interior computation but raises a raw error (raise ValueError at src/jacobian/math/finite_topology_spaces/operations.py:60) in the closure computation.
Impact: A request the tool accepts either returns a silently wrong set or aborts with a host-level failure rather than a mathematical answer.
Missing request-level validation of subset indices
SubsetRequest (src/jacobian/math/finite_topology_spaces/_models.py:14-18) declares subset: tuple[int, ...] with no cross-field validator tying the indices to space.points, unlike FiniteTopologicalSpace.require_well_formed which does validate preorder indices (src/jacobian/math/finite_topology_spaces/values.py:30-41).
Consequences for a 2-point space with subset=(5,):
interior(src/jacobian/math/finite_topology_spaces/operations.py:41-50) never inspects subset membership validity, returns a result computed against a nonsense subset.closureraisesValueError("subset index out of range"), which reaches the dispatcher as a backend exception.boundarycalls closure first, so it also raises.
AGENTS.md requires the request to encode the advertised mathematical domain and states that "A request the model accepts must return a typed domain result; mathematical inapplicability belongs in the request validator or the result, not in a raised backend exception."
Prompt for agents
SubsetRequest in src/jacobian/math/finite_topology_spaces/_models.py accepts arbitrary integers in `subset` without checking them against the space's point count. The kernel behaviour then diverges: interior() silently ignores invalid indices while closure() raises ValueError, so an accepted request can turn into a host exception (and boundary inherits that). Add an after-model validator on SubsetRequest that rejects (or otherwise handles as a typed result) indices outside 0..len(space.points)-1, and consider dropping the now-redundant raise inside operations.closure so kernels are total on validated input. Add tests covering an out-of-range subset for interior, closure, and boundary.
Was this helpful? React with 👍 or 👎 to provide feedback.
| @model_validator(mode="after") | ||
| def require_well_formed(self) -> Self: | ||
| if len(self.preorder) != len(self.points): | ||
| raise ValueError("preorder must have one row per point") | ||
| for row in self.preorder: | ||
| for idx in row: | ||
| if not 0 <= idx < len(self.points): | ||
| raise ValueError("preorder index out of range") | ||
| for i in range(len(self.points)): | ||
| if i not in self.preorder[i]: | ||
| raise ValueError("preorder must be reflexive") | ||
| return self |
There was a problem hiding this comment.
🔴 Finite topological spaces are accepted with an inconsistent point ordering, producing wrong interiors and closures
The point relation supplied for a finite space is only checked for reflexivity (require_well_formed at src/jacobian/math/finite_topology_spaces/values.py:30-41) and never for transitivity, so a relation that is not a real topology is accepted and every derived answer is silently wrong.
Impact: Users can get an "interior" that is not actually open and a "closure" that is not actually closed, with no error reported.
Alexandrov correspondence requires a preorder, not just a reflexive relation
The model documents preorder as a specialization preorder and the kernels rely on preorder[i] being the minimal open neighbourhood of i (src/jacobian/math/finite_topology_spaces/operations.py:32-64). The Alexandrov correspondence between finite topologies and preorders needs reflexivity and transitivity. With, e.g., points=("a","b","c") and preorder=((0,), (0,1), (1,2)), the validator accepts the input, but the family {preorder[i]} is not closed under the topology axioms: closure({0}) and interior results no longer satisfy their defining invariants (interior is not a union of minimal neighbourhoods of its own points).
The pre-existing sibling domain validates the full topology axioms for its input (src/jacobian/math/finite_topology/values.py:24-44), which is the established convention here. AGENTS.md also requires that "The request must encode the advertised mathematical domain—bounds, positivity, completeness, non-degeneracy—not only the JSON shape."
| @model_validator(mode="after") | |
| def require_well_formed(self) -> Self: | |
| if len(self.preorder) != len(self.points): | |
| raise ValueError("preorder must have one row per point") | |
| for row in self.preorder: | |
| for idx in row: | |
| if not 0 <= idx < len(self.points): | |
| raise ValueError("preorder index out of range") | |
| for i in range(len(self.points)): | |
| if i not in self.preorder[i]: | |
| raise ValueError("preorder must be reflexive") | |
| return self | |
| @model_validator(mode="after") | |
| def require_well_formed(self) -> Self: | |
| if len(self.preorder) != len(self.points): | |
| raise ValueError("preorder must have one row per point") | |
| for row in self.preorder: | |
| for idx in row: | |
| if not 0 <= idx < len(self.points): | |
| raise ValueError("preorder index out of range") | |
| for i in range(len(self.points)): | |
| if i not in self.preorder[i]: | |
| raise ValueError("preorder must be reflexive") | |
| for i in range(len(self.points)): | |
| row_i = set(self.preorder[i]) | |
| for j in row_i: | |
| if not set(self.preorder[j]).issubset(row_i): | |
| raise ValueError("preorder must be transitive") | |
| return self |
Was this helpful? React with 👍 or 👎 to provide feedback.
| """Supported native finite topological space API.""" | ||
|
|
||
| from jacobian.math.finite_topology_spaces.operations import ( | ||
| boundary, | ||
| closure, | ||
| continuous_check, | ||
| from_preorder, | ||
| interior, | ||
| kolmogorov_quotient, | ||
| minimal_neighbourhoods, | ||
| specialization_preorder, | ||
| ) | ||
| from jacobian.math.finite_topology_spaces.values import ( | ||
| FiniteTopologicalMap, | ||
| FiniteTopologicalSpace, | ||
| ) | ||
|
|
||
| __all__ = [ | ||
| "FiniteTopologicalMap", | ||
| "FiniteTopologicalSpace", | ||
| "boundary", | ||
| "closure", | ||
| "continuous_check", | ||
| "from_preorder", | ||
| "interior", | ||
| "kolmogorov_quotient", | ||
| "minimal_neighbourhoods", | ||
| "specialization_preorder", | ||
| ] |
There was a problem hiding this comment.
🟡 New finite topological space module is not reachable from the public math namespace
The new finite topological space package publishes a public API but was never added to the list of exported math domains (__all__ at src/jacobian/math/__init__.py:31-57), so it is missing from the supported native surface that every other domain with a public API appears in.
Impact: Callers browsing the supported native API will not see the new space operations at all.
Registration inconsistency across domains
src/jacobian/math/finite_topology_spaces/__init__.py:1-29 declares an explicit public __all__ and the PR adds tests/math/finite_topology_spaces/test_public_api.py, matching the convention of every other native domain. However, jacobian/math/__init__.py still imports and exports only the previous 25 domains, and tests/math/public_api/test_namespace.py:18-44 pins that exact list — every other domain that has a test_public_api.py is in it.
AGENTS.md states native public functions belong under jacobian.math as part of the native Python API contract; leaving the domain out of the root namespace makes the new API effectively unadvertised.
Prompt for agents
The new package src/jacobian/math/finite_topology_spaces declares a public native API (__init__.py with __all__ and an owner-local tests/math/finite_topology_spaces/test_public_api.py), but jacobian/math/__init__.py does not import or export it, and tests/math/public_api/test_namespace.py pins ROOT_MATH_DOMAINS to the old list. Either register the domain in the root namespace (import + __all__ entry, and update ROOT_MATH_DOMAINS in the namespace test), or, if the domain is intentionally catalog-only like posets, drop the public __all__ from its __init__ and the owner public-API test so the convention stays consistent.
Was this helpful? React with 👍 or 👎 to provide feedback.
| } | ||
|
|
||
|
|
||
| TOPOLOGY_SPACE_OPERATIONS: tuple[MathTool[Any, Any], ...] = ( |
There was a problem hiding this comment.
🔍 PR bundles an entire unrelated topology domain alongside the advertised number-theory work
The PR title and description cover only the four integer multiplicative normal-form operations, but the diff also introduces a complete new catalog domain (src/jacobian/math/finite_topology_spaces/ with 5 new public operations, admission rows, snapshot fragment, and tests) and bumps the catalog counts in tests/catalog/test_admission.py:30-37 by 9 rather than 4. CONTRIBUTING.md asks that each change stay focused on one outcome; more importantly, the topology domain has not been described or justified anywhere in the PR, so it will not get the review attention its new public contracts need. Worth confirming whether it was included accidentally.
Was this helpful? React with 👍 or 👎 to provide feedback.
| OperationAdmission( | ||
| "topology.finite.continuity_check.compute", | ||
| AdmissionDecision.KEEP, | ||
| "exact continuity check via specialization preorder monotonicity", | ||
| ), |
There was a problem hiding this comment.
🔍 New topology operations overlap with the existing finite_topology domain
jacobian.math.finite_topology already ships interior, closure, specialization_preorder, is_t0, and is_continuous (src/jacobian/math/finite_topology/__init__.py:1-37) plus the public catalog operation topology.is_continuous.compute (src/jacobian/math/finite_topology/_admission.py:18-22). The new domain adds topology.finite.continuity_check.compute, topology.finite.interior.compute, and topology.finite.closure.compute with a different (preorder-based rather than open-set-based) representation. The admission rationales for the new rows claim distinctness but do not address the overlap, and agents will now see two continuity checks with incompatible input shapes. Consider consolidating into the existing owner or documenting why two representations coexist.
Was this helpful? React with 👍 or 👎 to provide feedback.
morluto
left a comment
morluto
left a comment
There was a problem hiding this comment.
Review verdict: arithmetic kernels are correct, but this head is blocked by stacked unrelated code and an unbounded factorization contract
I independently checked the four number-theory formulas. They are correct:
- maximal perfect-power exponent is the gcd of prime exponents, reduced to its largest odd divisor for negative inputs;
- k-free decomposition uses
e=qk+rwith0 <= r < k; - squarefree decomposition uses exponent parity;
sqrt(n)=s*sqrt(d)uses the same canonical parity split.
This is structurally stronger than #2009: it distinguishes zero/units/nonzero cases more accurately, replays reconstruction, and does not publish a duplicate squarefree-part alias. However, the current PR still has three merge blockers.
1. The branch contains the whole finite-topology PR
The current diff adds finite_topology_spaces and its catalog entries in addition to these number-theory operations. That inherited code accepts reflexive but non-transitive relations as “preorders,” along with the other blockers documented on #2037. This PR therefore currently reintroduces a known-invalid mathematical value and cannot be reviewed/merged as an isolated implementation of #1893. Rebase onto main (or onto a corrected/merged #2037) so this PR contains only its owned domain.
2. A 256-digit string is not a factorization-work bound
Every accepted nontrivial request invokes complete sympy.factorint. A 256-digit semiprime is a valid request but not a bounded routine operation; the decimal-length cap does not make factorization complete within any defensible resource envelope. Either use a much smaller, measured bit bound suitable for complete factorization, or expose a bounded factorization status/budget and do not promise total exact completion for every accepted 256-digit integer.
3. Result values replay reconstruction but not canonical normal-form invariants
The validators still accept noncanonical witnesses. Examples:
64 = 8^2passes reconstruction even though exponent 2 is not maximal;16 = 1^2 * 16passes the squarefree result reconstruction though16is not squarefree;- radical
(coefficient=1, radicand=12)reconstructs 12 and can be classified irrational even though it is not normalized; - k-free rows do not require
remainder < k, and no validator binds quotient/remainder rows to a complete prime factorization; - parity rows do not require
parity == exponent % 2; - zero/unit perfect-power variants forbid base/exponent but may still carry contradictory factor rows or
is_nontrivial_perfect_power.
Make the result constructors enforce maximality, squarefreeness/k-freeness, row consistency, sign/canonical base rules, and absence of inapplicable fields—not only reconstruction.
Overlap decision
#2009 publishes the same substantive operation IDs, so both cannot merge. After removing the stacked topology changes and tightening the work/result invariants, this PR is the preferable base because its variants and schemas are closer to the actual mathematics. Reuse the repository's canonical integer abstraction rather than creating a parallel canonical-string contract where possible.
Deep review summaryVerdict: REQUEST CHANGES — the arithmetic formulas are correct, but the factorization contract is computationally unbounded and this head contains unrelated stacked changes. I independently checked the four number-theory constructions:
The main blocker is boundedness. Every operation requires complete factorization via SymPy This PR head also contains unrelated finite-topology changes and their outstanding defects. Split or rebase the PR onto the intended base so the normal-form work can be reviewed and merged independently rather than carrying another domain's blockers and catalog-count churn. Strengthen the result invariants as well: reconstruction alone does not prove that the reported cofactor is k-free, the squarefree part is squarefree, or the factor rows are complete. Replay those defining predicates in validators/property tests. |
1 participant
Summary
Implements issue #1893: integer multiplicative normal forms.
Adds four exact, typed, composable operations for canonical integer multiplicative normalization:
integer.perfect_power.profile.computeReturns the maximal perfect-power profile of one integer. Structural variants for zero (
ZERO), positive unit (POSITIVE_UNIT), and negative unit (NEGATIVE_UNIT) since they have no finite maximal exponent. For nonunits (NONUNIT), computes the canonical baseband maximal exponentesuch thatb^e = n, using the GCD of all prime exponents (restricted to the odd part for negative integers). Includes the complete prime-exponent factorization andis_nontrivial_perfect_powerflag.integer.k_free_decomposition.computeReturns the canonical k-free decomposition
n = a^k * cwherea >= 1,chas the sign ofn, and no prime to the k-th power divides|c|. Includes per-prime quotient/remainder rows. Zero uses a closedZEROvariant.integer.squarefree_decomposition.computeThe direct
k=2specialization:n = s^2 * dwheres >= 1,dhas the sign ofn, and|d|is squarefree. Includes exponent parity rows. Distinguishes the signed squarefree part from the prime-support product (radical).quadratic_radical.positive_integer.normalize.computeNormalizes
sqrt(n) = s * sqrt(d)for nonnegative integern, wheres >= 0,d >= 1is squarefree, ands^2 * d = n. Classifies asZERO,RATIONAL_INTEGER(whend=1), orIRRATIONAL_QUADRATIC(whend>1).Design choices
factorintas the private computational backend for prime factorization. Jacobian owns all public types and semantics.StrictModelfor all request/result models with built-in reconstruction validators (b^e = n,a^k * c = n,s^2 * d = n).integer.compute.nth_rootandinteger.compute.prime_factorizationfor cross-checks, not reimplementation.Test coverage
66 tests covering:
72 → d=2, not 6)sqrt(0)=0,sqrt(12)=2√3,sqrt(72)=6√2,sqrt(144)=12Closes #1893
Continue this on Linzumi