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

Interface to GLPK #437

Closed
jo-37 opened this issue May 14, 2023 · 12 comments
Closed

Interface to GLPK #437

jo-37 opened this issue May 14, 2023 · 12 comments

Comments

@jo-37
Copy link
Contributor

jo-37 commented May 14, 2023

From my wish list: Are there any chances to have an interface from PDL to GLPK as easy as in Octave? See https://www.gnu.org/software/glpk/ and https://docs.octave.org/latest/Linear-Programming.html

@mohawk2
Copy link
Member

mohawk2 commented May 28, 2023

There certainly are! You can start with one of the GSL modules (that live in Libtmp in the main PDL repo), copy that. A quick look at the glpk.h suggests you'd need a Perl constructor for the glp_prob data structure, then make the various functions that take that as the first param be methods in a package called something like PDL::GLPK, with a fairly literal translation of the C interface.

If you need help, please email one of the PDL mailing lists!

@jo-37
Copy link
Contributor Author

jo-37 commented May 28, 2023

Thanks for encouraging! Did some investigations in the meantime, but I'm not sure about the proper path. The GLPK interface is rather complex, but re-inventing the Octave interface doesn't seem to be a good idea neither. Need some time to think it over. There is an outdated Math::GLPK module that I couldn't get to work and there is Inline::Octave where its author gave up to bind liboctave with perl.

@mohawk2
Copy link
Member

mohawk2 commented May 28, 2023

If you like the Octave interface, then mimic that instead! If I were doing it myself, I'd start either with the OO approach outlined above, or a very literal translation of the C interface, like the GSL binding, but I'm not, so you do what you feel suitable.

@jo-37
Copy link
Contributor Author

jo-37 commented Feb 12, 2024

After some PP warmup:
Wrote a working prototype. It was not as complicated as expected. Octave comes with a C-function that is free from any Octave dependencies and that orchestrates all the required GLP functions. Just needed some glue on the PP-side and on the Perl side.

Configuration options are still missing and the function cannot broadcast.

I would extend this prototype to a full-fledged module if there were some principal acceptance for this stuff. It would be great if an PDL expert could take a look at the prototype.

@jo-37
Copy link
Contributor Author

jo-37 commented Feb 15, 2024

Added configuration parameters and documentation.

My question to pdl-porters: May I publish this module unter the name PDL::Opt::GLPK at CPAN?

@mohawk2
Copy link
Member

mohawk2 commented Feb 20, 2024

On behalf of pdl-porters: yes, absolutely! I have some design/implementation thoughts I'll deal with below. But instantly, to go on CPAN you're going to need to ensure the user has GLPK installed, probably with Devel::CheckLib. See Basic/Core/Makefile.PL for an example. You might consider making an Alien::GLPK to make this easier.

@mohawk2
Copy link
Member

mohawk2 commented Feb 20, 2024

More detailed thoughts.

I don't understand why glpk_aio.cc is C++? GLPK is in ANSI C, and I don't see any use of C++ there. I suspect it's because Octave did that? Compiling C++ with CPAN modules means you need to use ExtUtils::CppGuess, and I doubt you need that here.

This snippet caught my eye:

			if (!SvROK($COMP(param))) {
				$CROAK("'param' is not a reference");
			}
			SV *ref = SvRV($COMP(param));
			if (SvTYPE(ref) != SVt_PVHV) {
				$CROAK("'param' is not a hash reference");
			}

There are a number of ways to deal with this. The smallest is to do this error-check in HdrCode. I think a good way to go from Perl hashref to a C struct is to have a (large, here) number of OtherPars, and pass each of them in your PMCode, e.g.:

_glpk_int(..., @$params{qw(msglev dual...)});

I've had in mind for a while to expand OtherPars to automatically handle a hashref. For your purposes you'd also need to pass the whole "params" struct into the C function, to save on redundant coding.

Your glpk C function should probably return a pdl_error struct, with your error() function using something appropriate. You'd need to make the errnum a return parameter, i.e. int *. A further approach might be just to inline all of your wrapper function into your Code, which should simplify things a bit, including by not needing your own C struct. Then you could just throw errors with $CROAK. By the way, if you're doing file I/O, you should probably forbid broadcasting, unless you make the output filenames per-broadcast in some fashion.

@jo-37
Copy link
Contributor Author

jo-37 commented Feb 20, 2024

Thank you for your suggestions! Firstly, the function glpk in glpk_aio.cc is a verbatim copy from the Octave project. So it was not my decision to make it C++. The whole code in this module is just glue between Perl and the existing C++ function, and most of the glue is ported from C++ (using Octave classes) to C.
Regarding the OtherPars hash: I don't see a problem with handing the hashref into the C function. The corresponding C struct is mandatory as it is an input parameter to the Octave code.
Error handling is still in progress. I need to dig a bit deeper into the matter as this is my first PP resp. XS project. I'll try to understand your suggestions :-)

I'm not sure if I shall publish the module to CPAN in its current state. At least it is usable and documented. Reworking the error handling would change the interface, though.

@mohawk2
Copy link
Member

mohawk2 commented Feb 20, 2024

Thank you for your suggestions! Firstly, the function glpk in glpk_aio.cc is a verbatim copy from the Octave project. So it was not my decision to make it C++.

You've copied it from there, but now it's in your repo. That means you can change it. It doesn't have any C++ in. Please try deleting the extern "C" { stuff, renaming the file (and changing the reference in Makefile.PL), and seeing what happens.

Regarding the OtherPars hash: I don't see a problem with handing the hashref into the C function. The corresponding C struct is mandatory as it is an input parameter to the Octave code.

You can change the Octave code, because it's your code now. There isn't a problem as such, but conceptually I like to keep Perl stuff out of Code sections so that transforms (or PDL "operations", same thing) could be used in non-Perl contexts without change. See #358

Error handling is still in progress. I need to dig a bit deeper into the matter as this is my first PP resp. XS project. I'll try to understand your suggestions :-)

If you have questions, please ask!

I'm not sure if I shall publish the module to CPAN in its current state. At least it is usable and documented. Reworking the error handling would change the interface, though.

I'd say since it belongs entirely to you, and is so far only used by you, then any interface changes are fine?

@jo-37
Copy link
Contributor Author

jo-37 commented Feb 22, 2024

Thanks for your suggestions, that led me to some changes:

  • Stepped back from C++ to C with glpc_aio. My intention was to be able to drop in an updated version from Octave without changes. I realized this was a silly idea.
  • Adjusted the C structure to include all necessary parameters. I still do not see an advantage in not performing the hash-to-struct transformation within C. The parameters match one-to-one by name between the hash and the struct. A "copy-by-name" operation seems possible with C only, as I solved it with the SETPARM macro.
  • Moved the type check for the parms hash ref into the PMCode block. The suggestion to move it into the HdrCode block did not word because the HdrCode block has no effect if a PMCode block is in use. (Didn't find any hints about this in the docs.)
  • Removed the errnum from the output arguments by translating glp error numbers to their textual representation and using these for the PDL_err message.
  • Published PDL::Opt::GLPK

Thank you very much for your support! May I contact you if I have further questions? (Reopen the issue?)

I'd like to close this issue as the basic goal was achieved.

@mohawk2
Copy link
Member

mohawk2 commented Feb 22, 2024

I support you leaving it open if you'd like, but also if you'd like to close it.

  • I don't think it's a silly idea to want to drop-in Octave code, but the cost of supporting C++ here would to me have outweighed any likely benefit
  • Remember that XS is C, and that the hash-ref is an exclusively Perl thing. In a perfect world, the PDL binding of this would work when PDL is made available as a Python library. I think using a macro to solve this sort of thing is a great idea, specifically here an "X macro" (see https://www.perlmonks.org/?node_id=11151470). Another way to deal with this would be to move your C calling code into the Code section, as already advocated, because then you could use Perl code to generate the C struct, the parsing code, and/or the OtherPars

The best way to get support would be to join one or both of the pdl-{general,devel} mailing lists (https://pdl.perl.org/?page=mailing-lists), to reach a wider base of people with undoubtedly more knowledge than me.

Finally, HdrCode should either work when there's a PMCode, or be documented as not doing so (but the first is better). Feel like opening an issue so that doesn't get forgotten? Also, feel like having a go at making PDL::PP actually support that? I don't think it would be very tricky.

@jo-37
Copy link
Contributor Author

jo-37 commented Feb 23, 2024

Remember that XS is C, and that the hash-ref is an exclusively Perl thing.

Got it.
Here I have competing goals: Setting param defaults and range checking are a core function of this module that IMHO belong into the Code block. OTOH the Code block shall not deal with PDL.

Maybe I found a solution: Moving the PDL dependency into a a pp_addhdr block. Or does this just hide the problem?

Regarding HdrCode vs. PMCode: I'll take a look at it.

I'm going to subscribe the PDL mailing lists and think this issue shall be closed.

Once again: Many, many thanks for your great support!

@jo-37 jo-37 closed this as completed Feb 25, 2024
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

No branches or pull requests

2 participants