Macaulay2 / M2

The primary source code repository for Macaulay2, a system for computing in commutative algebra, algebraic geometry and related fields.
https://macaulay2.com
330 stars 226 forks source link

Added Normaliz as a linked library #3313

Closed mahrud closed 2 days ago

mahrud commented 1 week ago

@d-torrance the last commit needs work for getting autotools to find and link with Normaliz. Could you help with this?

d-torrance commented 1 week ago

Will do!

d-torrance commented 1 week ago

Just pushed a commit updating the autotools build. It's not quite working yet when we're building normaliz, but it should work when it already exists on the system.

mahrud commented 1 week ago

The check for libnormaliz on macOS seems to be failing because of -leanticxx. Is this necessary on Ubuntu? I'll give it a try without it.

d-torrance commented 1 week ago

I needed -leanticxx on my home desktop running Ubuntu 24.04, but I just tried on my laptop (still running 22.04) and the little test program linked just fine without it. I think it depends on whether libnormaliz was compiled with or without eantic support -- there's some eantic stuff in libnormaliz/general.h.

mahrud commented 1 week ago

Interesting. We don't need antic for now, but seems like Nauty is needed also:

Undefined symbols for architecture x86_64:
  "_densenauty", referenced from:
      libnormaliz::nauty_result<long> libnormaliz::compute_automs_by_nauty_Gens_LF<long>(libnormaliz::Matrix<long> const&, ...

When building it ourselves we certainly don't need either.

mahrud commented 1 week ago

That seems to have done the trick. At some point making sure that libnormaliz builds successfully would be good, but I think this is good for now.

ggsmith commented 1 week ago

I have a few questions:

mahrud commented 1 week ago
  • In FourierMotzkin: I don't understand some of the changes the documentation node "Finding the facets of the cyclic polytope". Why compute the vertices of the cyclic polytope, rather than just constructing them explicitly? The original code seems more accessible to me. Perhaps the best option is to include both by verifying that the more elementary construction coincides with the one obtain from the cyclicPolytope construction.

I changed this because of the conflict in redefining cyclicPolytope from Polyhedra, but in a new push I just expanded the documentation to show the explicit construction.

  • In NormalToricVarieties: as I understand it, the methods isVeryAmple and latticePoints now both depend on the Polyhedra package. The earlier code was explicitly designed to avoid this. Can this code not be altered to call rawHilbertBasis directly? If not, then I would much prefer to wait to make this change until all the Normalize routines are available.

The Polyhedra methods latticePoints and hilbertBasis are a little obfuscated but are not doing anything fancy, only post-processing the output of rawHilbertBasis. Yes, the toric variety methods isVeryAmple and latticePoints can be altered to call rawHilbertBasis directly, but I'm against duplicating the raw post-processing code in Polyhedra. Specifically, computing lattice points of a polytope using a Hilbert basis computation is a piece of code that I think should be defined in one efficient routine rather than several places.

I should add that with this PR, NormalToricVarieties doesn't import the package Normaliz anymore, so I don't understand what would change when all Normaliz routines are available. Do you have another suggestion?

  • In NormalToricVarieties: in the documentation nodes where you removed links to the (isSmooth, NormalToricVariety) page, did you check that this link already appears in the SeeAlso section?

I did not, but in a push just now I changed them all to link to the correct node.

  • In NormalToricVarieties/Tests.m2: Why did you change the sort to a set?

I'm actually not sure why I changed it so drastically, I just changed it back, but importantly rawHilbertBasis from Normaliz and 4ti2 have different output orders, which actually results in a desingularization with different order of rays in this example, so in the maximal cones 3 and 5 will be swapped.

mahrud commented 1 week ago

@jkyang92 could you review the parts of this PR in the engine? I'm not confident if I dealt with memory matters safely.

@mikestillman could you enable requesting reviews from anyone? Currently I can't ask Jay to leave a review because it's only limited to this team.

mikestillman commented 1 week ago

@mahrud Do you know how I enable requesting reviews from anyone? I don't immediately see where to do so...

mahrud commented 1 week ago

I think Settings > Moderation Options.

mikestillman commented 1 week ago

@mahrud, on github repo settings for M2, I look at

Restrict users who are permitted to approve or request changes on pull requests in this repository.

[]Limit to users explicitly granted read or higher access
When enabled, only users explicitly granted access to this repository will be able to submit pull request reviews that "approve" or "request changes". All users able to submit comment pull request reviews will continue to be able to do so.

and there is a checkmark box, which is unchecked, for this option. This seems to be the correct setting, but perhaps it should be checked?

jkyang92 commented 1 week ago

@mahrud The memory management is fine. I added a comment on a minor unrelated thing I noticed.

DanGrayson commented 6 days ago

Could you resolve the merge conflicts?