Rename set_from_* methods to set#63
Open
d-torrance wants to merge 4 commits into
Open
Conversation
Removes the redundant _from_X suffix from ARing conversion methods: set_from_long, set_from_mpz, set_from_mpq, set_from_double, set_from_doubles, set_from_BigReal, set_from_BigReals, set_from_BigComplex, set_from_Interval, set_from_ComplexInterval, and set_from_int (in tower/dpoly). The source type is already encoded in the argument, making the suffix redundant. As a prerequisite, removes set_from_complex_double (CC/CCC/CCi) and set_from_complex_mpfr (CCC), which were duplicate methods with identical implementations to set_from_doubles and set_from_BigReals respectively, differing only in return type (bool vs void). Retains has_set_from_* names for the concept detectors in aring-translate.hpp (e.g. has_set_from_double, has_set_from_mpq), reverting a mechanical has_set_* rename from an earlier draft. The from_ infix is informative since these concepts detect set() overloads for specific source types. Fixes ARingZZ::invert to call fmpz_set directly rather than the ambiguous set(ElementType&, ElementType) call, which becomes ambiguous with set(ElementType&, long) after the rename since ARingZZ::ElementType = fmpz = long. Adds set(ElementType&, int) to ARingRR, ARingRRR, ARingRRi, ARingCC, ARingCCC, ARingCCi, and ARingQQGMP, which all have both long and double set overloads. Without it, set(elem, int_literal) is ambiguous since int converts to both long and double. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The six free template functions get_from_double, get_from_BigReal, get_from_Interval, get_from_complex_double, get_from_BigComplex, and get_from_ComplexInterval are renamed to try_set. The name better describes what they do: attempt R.set(elem, val) and return false if the ring doesn't support that overload. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rategy) ARingGFM2 stores field elements as table indices (int), not integers. set(elem&, elem) and set(elem&, long) had the same semantics as copy() and fromZZTable() respectively, but after renaming set_from_long to set(long), calls like ring().set(result, 1) resolved to the elem overload (exact match on int), setting a raw index instead of converting the integer through the GF table. Fix: remove set(elem&, elem) from ARingGFM2 so that set() always means "convert from integer". Replace all element-copy uses of ring().set() with ring().copy() throughout the matrix and ring infrastructure: - dmat-lu.hpp, dmat-lu-inplace.hpp: LU decomposition (original fix) - mat-elem-ops.hpp: DMat getEntry, setEntry, and submatrix helpers - mat-arith.hpp: matrix copy/assignment and transpose - mat-util.hpp: matrix concatenation - smat.hpp: sparse matrix lead_row and vec_set_entry - dmat-lu-zzp-flint.hpp: solution extraction - aring-glue.hpp: ConcreteRing::copy() virtual method ConcreteRing::copy() now uses Element b(*R) + R->copy(b.value(), a) instead of R->set(b, a), using b.value() to give copy() an unambiguous ElementType& argument so template deduction resolves to the correct per-ring copy overload. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Like GFM2, elements are ints, but representing exponents instead of actual integers. We update set() to promote the integer and use copy() in a few unit tests where we want to copy the exponent instead.
Collaborator
Author
|
Builds passed! https://github.com/d-torrance/M2/actions/runs/28074635112 |
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
1 participant
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.
This was originally part of #62, but I split it off.
We rename the various
set_from_*methods (likeset_from_long,set_from_double, etc.) to justset. The method names were redundant since the thing we're setting from is clear from the second argument. This point come up in our C++ factoring group discussion last Friday.Similarly, the various
get_from_*methods are renamed totry_set, since that's what they do -- they try callingsetand return false if they can't.One little wrinkle --
GFM2(the "New" strategy forGF) usesintfor its elements, but not how you'd expect. 1 isn't actually 1, buta^1, whereais a generator for the multplicative group of units. So there was some ambiguity -- shouldset(x, 1)mean "set x to be 1" or "set x to be a^1"?We made the decision that, for consistency with all the other rings where setting from an
intmeans promoting fromZZ, it should be the former. That messed up a few algorithms that calledsetto mean the latter thing, so we switched them to usecopyinstead.Draft for now to test the builds