Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

Upstream NConvex patches so that we can use an official version #69

Open
fingolfin opened this issue Oct 7, 2021 · 1 comment
Open

Comments

@fingolfin
Copy link
Member

Right now, JToric uses a patched version of NConvex in order to allow the JConvex GAP package to override the NConvex code calling Normaliz/Cddlib, by code which calls Polymake (through Julia / Polymake.jl) instead.

We don't want this on the long run. Rather, we want to work with @kamalsaleh and @mohamed-barakat to get changes into NConvex that allow us to use that again. There are several ways to do this:

  1. we could merge JConvex into NConvex and then NConvex could be somehow configured to use either Normaliz/Cddlib, or to use JConvex
  2. we could change NConvex to always load (even without NormalizInterface/CddInterface present); it'd always load its general infrastructure, but would load the code specific to normaliz/cdd if those are present. (That's effectively what @HereAround's patched version currently does)
  3. one could move the common code to a separate package (perhaps reactivate the Convex package for that?) and then one can have multiple "providers" of actual implementations.

Option 3 sounds the cleanest on paper, but it's also most work, I think, and it's not clear who would make the decision which "provider" of functionality to actually load, and when. This is solved better with 1 and 2. The drawback of 1 is that we are currently still changing JConvex a lot; and ideally I think we should have it inside the JToric.jl repository, so that we can change it in lock step with the rest of JToric. So from that point of view, option 2 would be best, except if not done carefully, it'd degrade the user experience for people using "regular" GAP (for those ideally nothing would change).

So here's a slightly crazy idea: as a slight variant of approach (2), the PackageInfo.g of NConvex could be changed to look something like this:

# if running GAP under Julia and JConvex is around, then use that
if IsPackageMarkedForLoading("JuliaInterface", "") and TestPackageAvailability("JConvex") then
   nconvex_needed := [ [ "JConvex"  ] ];
else
   nconvex_needed := [ [ "CddInterface", ">= 2020.06.24" ],
                           [ "NormalizInterface", ">= 1.1.0"  ] ];
fi;

SetPackageInfo( rec(
PackageName := "NConvex",
...
Dependencies := rec(
  GAP := ">= 4.9",
  NeededOtherPackages := Concatenation([ [ "AutoDoc", ">= 2018.02.14" ],
                           [ "Modules", ">= 0.5" ], 
                           [ "CddInterface", ">= 2020.06.24" ],
                           [ "NormalizInterface", ">= 1.1.0"  ]
                         ],
...
));
Unbind(nconvex_needed);
```
@kamalsaleh
Copy link

kamalsaleh commented Oct 8, 2021

I am fine with making NConvex loadable even if Nmz&Cdd are not present; and even if they are available and have been loaded as SuggestedOtherPackages, then the choice to load JConvex will override all methods to use Polymake.jl which is the desired behavior of loading JConvex. In fact, the old Convex package was also loadable without PolymakeInterface.

I would rather avoid using the following code because it creates a circular dependency and prioritize JConvex whenever available.

if IsPackageMarkedForLoading("JuliaInterface", "") and TestPackageAvailability("JConvex") then
   nconvex_needed := [ [ "JConvex"  ] ];
else
   nconvex_needed := [ [ "CddInterface", ">= 2020.06.24" ],
                           [ "NormalizInterface", ">= 1.1.0"  ] ];
fi;

If @mohamed-barakat is also fine and @HereAround confirms that the changes solve the problem, I would merge the commit
https://github.com/kamalsaleh/NConvex/tree/devel to homalg/master branch

On the long run, I think it is sensible to merge the two packages to a new package with a key to prioritize between Nmz/Cdd and Polymake, and maybe also call it again Convex. The old Convex was based on polymake and now this is achieved by JConvex.

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

No branches or pull requests

2 participants