Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Msolve #3366

Merged
merged 44 commits into from
Aug 7, 2024
Merged

Msolve #3366

merged 44 commits into from
Aug 7, 2024

Conversation

antonleykin
Copy link
Contributor

This introduces Msolve package, which is an interface to msolve developed mainly by @Martin-Helmer (with parsing routines in the kernel by @mikestillman).

@mikestillman
Copy link
Member

@d-torrance Is it possible to have the autotools build automatically install msolve? @mahrud I believe we can do this (or it is already done) on the cmake build? (Here, I mean the msolve executable).

@d-torrance
Copy link
Member

@d-torrance Is it possible to have the autotools build automatically install msolve?

Certainly! Would you like me to write the code to do this?

@mikestillman
Copy link
Member

@d-torrance That would be great if you could do so! Thanks!

@d-torrance
Copy link
Member

@mikestillman - See antonleykin#26

@mahrud
Copy link
Member

mahrud commented Jul 18, 2024

@d-torrance you could also just push here since it says "Maintainers are allowed to edit this pull request." Then we have one fewer merge commit ...

@d-torrance
Copy link
Member

Ah, indeed! Done.

@mahrud
Copy link
Member

mahrud commented Jul 18, 2024

I believe we can do this (or it is already done) on the cmake build?

Yes, it already happens.

@d-torrance d-torrance force-pushed the Msolve branch 2 times, most recently from 8fc56b2 to d8046ad Compare July 19, 2024 12:17
@d-torrance
Copy link
Member

I just pushed a couple more commits, adding the package to =distributed-packages (which should fix the broken link error), and installing the Ubuntu/brew msolve packages for the GitHub builds.

@mikestillman
Copy link
Member

@d-torrance @mahrud What versions are msolve at in ubuntu/homebrew/M2? Mohab reported that a bug I noticed (in version v0.6.5) appears to be fixed in v0.6.6.

@d-torrance
Copy link
Member

For Ubuntu, it depends on the release. Ubuntu 24.04 has 0.6.5, and 24.10 will ship with 0.6.6. See https://launchpad.net/ubuntu/+source/msolve. I can always update the version in the PPA (which covers LTS releases all the way back to 18.04) -- right now it's still at 0.4.9.

I'd forgotten that there was an 0.6.6 release -- it's not listed on the main msolve webpage, just their GitHub page. I should update the autotools commit to build that instead.

@d-torrance
Copy link
Member

d-torrance commented Jul 19, 2024

I think the Ubuntu build errors might be coming from the old version of msolve (0.4.9) in the PPA. I'm working on packaging the newest version (0.6.7, released earlier today) for Debian right now. I'll backport it for the PPA after that, and hopefully that will fix the builds.

@Martin-Helmer
Copy link
Contributor

I think the Ubuntu build errors might be coming from the old version of msolve (0.4.9) in the PPA. I'm working on packaging the newest version (0.6.7, released earlier today) for Debian right now. I'll backport it for the PPA after that, and hopefully that will fix the builds.

Yes, I would expect the tests in the Msolve M2 package to fail with any msolve version before 0.6.4

@d-torrance
Copy link
Member

msolve 0.6.7 is now live on the PPA, so I've restarted the Ubuntu builds.

Comment on lines 186 to 188
if opts#"output type"=="rationalInterval" then return sols;
if opts#"output type"=="floatInterval" then return (1.0*sols);
if opts#"output type"=="float" then return (for s in sols list(for s1 in s list sum(s1)/2.0));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing "float"=RR and "floatInterval"=RRi, but what is "rationalInterval"? Also, isn't it easier for the user to do 1.0*sols if they want, and use midpoint for the "float" option?

Here are a few alternatives that I think would be more idiomatic here:

  1. split this function into msolveRealSolution, msolveRealIntervalSolution, and msolveRationalSolutionInterval (if this exists in M2)
  2. have this function always return sols, i.e. "rationalInterval" option, but have helper functions like midpoint for turning that into a float interval or float, which could also be used in other places idiomatically
  3. add a new input of type Ring to this function, which could be QQ or RR or RRi and cast to that ring.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong feeling here, but mostly the interval return might be a bit hard for people to read. But it makes sense for the default as it is still the most accurate answer.

I don't like suggestion 3 so much as expect feeding anything other than QQ or a finite field into msolve is going to create infinite problems.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the default should be the most accurate, i.e. sols. Why is it not returning RRi interval objects?

For option 3, it should definitely throw an error if the ring is anything other than the handful of fields we can handle.

Copy link
Contributor

@Martin-Helmer Martin-Helmer Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not returning RRi objects mostly since I am not familiar with these.

The default return is what msolve would return, unedited. So it is intervals of rationals.

I have left this as is in my most recent update, but if others want to change it that is also fine with me.

);
msolveLeadMonomials=method(TypicalValue=>Matrix, Options=>{"level of verbosity"=>0, "number of threads"=>maxAllowableThreads});
msolveLeadMonomials(Ideal):=opt->(I)->(
if not inputOkay(I) then return 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, throwing an error. We can add hooks for leadMonomial and add this as a hook, which can be specified using the Strategy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, should this be called msolveLeadTerm instead, or am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

msolve always returns monic monomials with the GB option which computes the lead term. So this seems like a reasonable name?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't the lead terms of a Grobner basis over a field are always monic? e.g. in M2 leadTerm gb I always returns monic monomials also, so I think it's important to align the methods with M2's native ones.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leadTerm groebnerBasis I
and
leadTerm I
both return a matrix with the coefficients included (and are not necessarily monic).

And for me leadTerm gb I does not nessarily return monic monomials (of course you can just throw away the coefficients, but that is not what it does, and leadTerm seems more correct when you keep the coefficients... the ideal is of course the same either way).

Anyway, to me the msolve GB is closer aligned to the "groebnerBasis" command rather than the "gb" command, which was why I opted to do it this way, i.e. if it was msolveLeadTerm I would want its output to match leadTerm groebnerBasis I, but it won't.

Also msolve only has grevlex and block grevlex (as an elimination order) defined, so in some sense I am not sure we want to align it too closely to the M2 commands since it has more limited capabilities in some sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah interesting, it seems like this is actually field dependent:

i21 : R = ZZ[x,y]; leadTerm gb ideal (2*x+3*y)

o22 = | 2x |

              1      1
o22 : Matrix R  <-- R

i23 : R = ZZ/5[x,y]; leadTerm gb ideal (2*x+3*y)

o24 = | x |

              1      1
o24 : Matrix R  <-- R

@mikestillman is this a bug?

msolvePresentAndModern := msolvePresent -- and match("^[0-9.]+$",msolveVersion) and msolveVersion >= msolveVersionNeeded

newPackage(
"Msolve",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be capitalized as MSolve instead? e.g. see the Sage interface.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the msolve team always seems to call it msolve or MSOLVE.... so... I think there is no canonical choice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other alternative is msolveInterface (following gfanInterface). Eventually, it might be a good idea to have a subdirectory packages/interfaces which contains all interface packages named exactly like the software, e.g. gfan.m2 msolve.m2 etc.

Thoughts @mikestillman?

Copy link
Member

@mahrud mahrud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you shorten these headlines?

[1/1] Installing package Msolve
 -- warning: document: documentation headlines must be less than 100 characters long:
 --   "Macaulay2 interface for msolve; computes real solutions, Groebner basis (in GRevLex), saturations, and elimination ideals."
 -- warning: document: documentation headlines must be less than 100 characters long:
 --   "Uses symbolic methods to compute a certified solution interval for each real solution to a zero dimensional polynomial system."
 -- warning: document: documentation headlines must be less than 100 characters long:
 --   "Uses symbolic methods to compute the rational univariate representation of a zero dimensional polynomial system."
 -- warning: document: documentation headlines must be less than 100 characters long:
 --   "Computes a Groebner basis for the saturation of an ideal by a single polynomial in GrevLex order; only works over a finite field."

From Package Writing Style Guide:

The "headline" of a documentation node should be a single brief phrase (not a complete sentence) and thus can be vague. It should not refer to variables by name, because it can appear in a menu, without the accompanying body of the documentation node.

@d-torrance
Copy link
Member

I've fixed the runProgram issues I mentioned above and also rebased this branch on top of development, resolving the conflicts.

Comment on lines +151 to +156
returnValue := (
if opts.Verbose
then run ("(((((" | cmd | "; echo $? >&4) | tee " | outFile |
") 3>&1 1>&2 2>&3 | tee " | errFile |
" >&5) 4>&1) | (read r; exit $r)) 5>&1")
else run (cmd | " > " | outFile | " 2> " | errFile));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works great, thank you!
For future though, I wonder if it would be best to add a c++ routine for this.

@mahrud
Copy link
Member

mahrud commented Aug 7, 2024

GitHub hid this message after the rebase, so quoting final thoughts from above:

  • perhaps renaming to msolveInterface.m2, which matches their own capitalization of msolve as well as the package gfanInterface.m2 is more appropriate.
  • I still think the optional input of msolveRealSolutions is strange, and instead it should return an RRi interval, which the user can take midpoint of, for instance.

@antonleykin
Copy link
Contributor Author

antonleykin commented Aug 7, 2024

GitHub hid this message after the rebase, so quoting final thoughts from above:

  • perhaps renaming to msolveInterface.m2, which matches their own capitalization of msolve as well as the package gfanInterface.m2 is more appropriate.
  • I still think the optional input of msolveRealSolutions is strange, and instead it should return an RRi interval, which the user can take midpoint of, for instance.

The majority of distributed "Interfaces" follow the pattern that @Martin-Helmer followed. I see no problem.

Capitalization is done the same way for nauty, bertini, topcom, ...

@mahrud
Copy link
Member

mahrud commented Aug 7, 2024

Okay. I'll merge this then, and maybe we can come back to msolveRealSolutions later.

@mahrud mahrud merged commit bf9ac3e into Macaulay2:development Aug 7, 2024
5 checks passed
@antonleykin antonleykin mentioned this pull request Aug 7, 2024
if msolveProgram === null then (
if not (options currentPackage).OptionalComponentsPresent
then ( printerr "warning: msolve cannot be found; ending"; end )
else ( printerr "warning: msolve found but its version is older than v" | msolveMinimumVersion;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line won't run, you need parentheses around the argument. (a bit late I know)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants