Msolve parser fix #3763
Msolve parser fix #3763
Conversation
…iles, checks pass for Msolve
|
It passes the main example that @Martin-Helmer mentioned here. Could you actually add this as a test? needsPackage "Msolve";
R=QQ[x..z,t]
K=ideal(x^6+y^6+x^4*z*t+z^3,36*x^5+60*y^5+24*x^3*z*t,
-84*x^5+10*x^4*t-56*x^3*z*t+30*z^2,-84*y^5-6*x^4*t-18*z^2,
48*x^5+10*x^4*z+32*x^3*z*t,48*y^5-6*x^4*z,14*x^4*z+8*x^4*t+24*z^2)
W1=msolveEliminate(R_0,K)
W2=eliminate(R_0,K)
sub(W1,R)==W2 |
| @@ -59,6 +58,41 @@ long readInteger(const std::string_view& str, size_t& begin_loc, size_t end_loc) | |||
| return result; | |||
| } | |||
|
|
|||
| // allocates memory to (and inits) an __mpz_struct | |||
| // void readInteger_mpz_t(mpz_t& result, const std::string_view& str, size_t& begin_loc, size_t end_loc) | |||
There was a problem hiding this comment.
What's the difference between this and readInteger_mpz_class?
| R = QQ[x..z,t] | ||
| K = ideal(x^6+y^6+x^4*z*t+z^3,36*x^5+60*y^5+24*x^3*z*t, | ||
| -84*x^5+10*x^4*t-56*x^3*z*t+30*z^2,-84*y^5-6*x^4*t-18*z^2, | ||
| 48*x^5+10*x^4*z+32*x^3*z*t,48*y^5-6*x^4*z,14*x^4*z+8*x^4*t+24*z^2) | ||
| errorDepth=2 | ||
| W1 = msolveEliminate(R_0, K, Verbosity => 1) | ||
| W2 = eliminate(R_0, K) | ||
| sub(W1, R) == W2 | ||
| assert(sub(W1, R) == W2) |
There was a problem hiding this comment.
Oh, never mind, it's already here ..
| class BasicPoly | ||
| { | ||
| public: | ||
| std::vector<int> mCoefficients; | ||
| std::vector<mpz_class> mCoefficients; |
There was a problem hiding this comment.
The function bytesUsed will need to change. We don't really need to do that before this PR is accepted, but it should be changed.
| @@ -40,7 +40,7 @@ void BasicPolyListStreamCollector::appendExponent(VarIndex index, Exponent expon | |||
| mValue[mCurrentPoly].mMonomials[mSizeEntryInMonomial] += 2; | |||
| } | |||
|
|
|||
| void BasicPolyListStreamCollector::appendTermDone(Coefficient coefficient) | |||
| void BasicPolyListStreamCollector::appendTermDone(const Coefficient& coefficient) | |||
| { | |||
| mValue[mCurrentPoly].mCoefficients[mCurrentTerm] = coefficient; | |||
| } | |||
There was a problem hiding this comment.
Is this correct? Does this correctly copy the element? I think it probably does, just want to flag that
| using VarIndex = int32_t; // TODO: these must match BasicPoly above. | ||
| using Exponent = int32_t; | ||
| using Component = int32_t; | ||
| using ModulusType = int32_t; |
There was a problem hiding this comment.
What characteristics do we want to use here? mathic uses roughly 16 bit primes, while msolve seems to want much larger primes. Are they still less than e.g. 2^31?
|
|
||
| if (sign == -1) coeff = -coeff; | ||
| result.mCoefficients.push_back(coeff); | ||
| result.mCoefficients.push_back(coeff); // do not clear(coeff) ! |
There was a problem hiding this comment.
I'm confused about this. the push_back is doing a copy? Why should we not clear it? I agree we don't need to, but would that invalidate the coeff that just got pushed on the vector of coefficients?
| @@ -184,7 +217,7 @@ void parseBasicPoly(const std::string_view& str, const IdentifierHash& idenHash, | |||
| { | |||
| throw parsing_error("expected a digit at position " + std::to_string(begin_loc)); | |||
| } | |||
| e = readInteger(str, begin_loc, end_loc); | |||
| e = readInteger_long(str, begin_loc, end_loc); | |||
There was a problem hiding this comment.
exponent vectors are ints. Are we silently truncating numbers to int size here?
| { | ||
| mpz_t tmp; | ||
| mpz_init_set_si(tmp, p); // Convert int p to mpz_t | ||
| mpz_mod(a, a, tmp); // a = a mod p (always non-negative) |
There was a problem hiding this comment.
Maybe we should keep an mpz_t characteristic here too, so this doesn't need to be done each time?
There was a problem hiding this comment.
There are a few things I will want to change in this PR, but it does fix the msolve reading large integers issue, and it doesn't mess up the mathicgb computations, so I say: let's take it. @antonleykin and I can talk through the thoughts I have about making it more future robust.
Thanks @antonleykin for fixing that!
Fixed the parser for
msolveoutput.Note:
intwithmpz_tandmpz_classwhere appropriatemathicgb(but we have no issues so far)