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

WIP: Refactoring and Enhancements Related to Parameters #139

Merged
merged 74 commits into from
Jan 2, 2022
Merged

Conversation

tkralphs
Copy link
Member

@tkralphs tkralphs commented Nov 23, 2020

We plan to merge this long-standing PR on 1/1/2022 as part of a larger strategy for refactoring Cbc and Clp.

CoinUtils is is currently undergoing some refactoring, including expansion of the helper classes and utilities that support sotring parameter values in both Clp and Cbc. This pull request is mainly for the purposes of documenting the changes being made so that other maintainers can review and follow along. The primary changes in this pull request are related to the files ClpParam.[hc]pp and ClpParamUtils.cpp. They implement the following general scheme.

Each parameters has a type, which can be queried and is one of

  • Double: Parameters whose value is a double
  • Integer: Parameters whose value is an integer
  • String: Parameters whose value is a string
  • Directory: For storing directory names, such as default locations for file types
  • File: For storing the locations/names of files
  • Keyword: Parameters that have one of several enumerated values identified by either strings or corresponding integer modes.
  • Action: Not parameters in the traditional sense, but used to trigger actions, such as solving an instance.

There are different constructors for each type, as well as setup functions for populating an existing parameter object with

  • Name
  • Short help string
  • Long help string
  • Limits on values (for error checking)
  • Defaults values
  • Display priority (to limit which parameters get displayed when users ask for help)

For keyword parameters, there is also a mechanism for creating a mapping between keyword value strings and the so-called "mode" value, which is an integer. The value of a keyword parameter can be set or retrieved either as a keyword string or as an integer mode. The modes may also be specified in enums.

There are separate get/set methods for each parameter type, but for convenience, there is also a single setVal() and getVal() method that is overloaded and can set the value of any parameter (the input/output is interpreted based on the known parameter type), e.g.,

param.setVal(0);

Each parameter object also has optional "push" (and "pull") function that can perform actions or populate related data structures upon the setting of a parameter value.

None of the methods in the class produce output directly. The get/set methods can optionally populate a string object that is passed in as an optional second argument. This is to allow the calling function to control output by piping the string to a message handler as appropriate or simply ignoring it if printing is not desired. This obviates the need to control printing internal to the functions themselves, which would require a separate parameter.

std::string message;
if (param->setKwdVal(value, &message)){
   messageHandler->message(CLP_GENERAL, generalMessages) << message << CoinMessageEol;
}

@tkralphs tkralphs marked this pull request as draft November 23, 2020 23:00
@tkralphs tkralphs mentioned this pull request Nov 23, 2020
25 tasks
@tkralphs tkralphs mentioned this pull request Mar 15, 2021
26 tasks
src/CoinParamUtils.cpp Show resolved Hide resolved
src/CoinParamUtils.cpp Outdated Show resolved Hide resolved

void
readInteractiveInput(std::deque<std::string> &inputQueue,
std::string prompt)
Copy link
Contributor

Choose a reason for hiding this comment

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

passing in a (possibly const) reference would safe a string copy. I won't comment that on the other functions in this file, I can see a pattern.

Copy link
Member Author

Choose a reason for hiding this comment

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

I assume you mean for passing in prompt? Yes, makes sense. I wasn't super careful about const, etc. in doing this. Wanted to get something working first, then clean up.


char c;
std::stringstream ss(field);
ss >> value;
Copy link
Contributor

Choose a reason for hiding this comment

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

if we can't use std::stoX due to c++98, can we at least kill this copy&paste by using a template (or redirecting to a "hidden" template)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you clarify this one? Do you mean the fact that this code snippet is repeated? I'm open to alternatives to anything here, I wouldn't exactly say I'm an expert on best practices in C++.

continue;
if (param->display() || hidden) {
std::string nme = param->matchName();
if (paramVec[i]->getDisplayPriority() || hidden) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Now the comment in line 941 is lying - if paramVec contains NULL, this line will dereference it.
Are params now supposed to be != NULL?

Copy link
Member Author

Choose a reason for hiding this comment

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

The way the parameter mechanisms of Clp and Cbc are designed, there shouldn't be any NULLs because parameters are indexed by an enum that starts at 0 and has no gaps. But it doesn't hurt to allow for this case, so I'll switch it back to the way it was previously (with the check for NULL). I'm not sure why I changed it to begin with, but I did initially change CoinParamVec to a vector of CoinParam objects, since every element needs to be allocated only at the beginning and deallocated only at the end and this seemed convenient. But I eventually went back to dynamically allocating the each CoinParam object for reasons I've forgotten now.

@@ -602,9 +726,6 @@ int matchParam(const CoinParamVec &paramVec, std::string name,
shortCnt = 0;

for (int i = 0; i < vecLen; i++) {
CoinParam *param = paramVec[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

if params are now != NULL, we might be able to use std::vector instead of std::vector<T*>, which would
1.) avoid accidential NULL
2.) new'ing every element in the vector (as seen in CbcParameters)
3.) deleting every element in the vector (as seen in CbcParameters).

vector[] returns references, so we could kill a lot of pointers and questions "can it be NULL" and "who owns it" are avoided/enforced by the language. I'm currently evaluating if it is actual doable or if there is a piece of code that needs a pointer

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, yes, I went around and about on all of this stuff when I was thinking through the whole thing and if you look at the history of the PR in CLP, you will see that I did do exactly as you said initially. At some point, I ran into an issue with doing that and switched everything back again. If I were a professional and this was my day job :), I would have more carefully documented why I made the switch. But you can see from the comment in this commit that I somehow decided we need std::vector<T*>.

With all that said, once things are up and running, I'm happy to discuss any of these sort of design decisions and revisit them to fine-tune. I was working on this in a vacuum initially. It's nice to have some sort of sounding board.

@CLAassistant
Copy link

CLAassistant commented May 14, 2021

CLA assistant check
All committers have signed the CLA.

@tkralphs tkralphs marked this pull request as ready for review December 30, 2021 19:29

#ifdef COINUTILS_HAS_GLPK
#include "glpk.h"
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Same as in Osi and Clp: This means that any code that includes CoinMpsIO.hpp needs to be able to find glpk.h when compiling. This shouldn't be necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

As I commented in Osi, I see that we may not need to include glpk.h here, but the includes are all in files that themselves call readGmpl, which means that in order to compile CoinMpsIO.cpp, we have to be pulling in a header that itself pulls in glpk.h. In other words, if glpk.h can't be found, then the build is going to break with or without this particular include.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do actually need this because readGMPL is implemented in CoinMpsIO (I had forgotten that).

Copy link
Member

Choose a reason for hiding this comment

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

The comment here is on CoinMpsIO.hpp, a header that is installed. So it isn't just about the build of CoinMpsIO.cpp, but also the build of any project that includes CoinMpsIO.hpp.

@@ -24,10 +24,10 @@ unitTest_SOURCES = \
unitTest.cpp

# List libraries to link into binary
unitTest_LDADD = ../src/libCoinUtils.la
unitTest_LDADD = ../src/libCoinUtils.la $(COINUTILSLIB_LFLAGS)
Copy link
Member

Choose a reason for hiding this comment

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

It shouldn't be necessary to repeat the flags used to link the CoinUtils lib here.


# Here list all include flags, relative to this "srcdir" directory.
AM_CPPFLAGS = -I$(srcdir)/../src
AM_CPPFLAGS = -I$(srcdir)/../src $(COINUTILSLIB_CFLAGS)
Copy link
Member

Choose a reason for hiding this comment

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

The compiler flags for building the CoinUtils lib shouldn't be necessary to build the CoinUtils test, since that should just be a user of the library. Also COINUTILSLIB_CFLAGS includes -DCOINUTILS_BUILD, if I remember correctly, so that the full config.h will be included. One shouldn't have this when compiling the source of the test.

Copy link
Member Author

@tkralphs tkralphs Jan 1, 2022

Choose a reason for hiding this comment

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

It seems that we actually need this, too. When I remove it, the build fails because it can't find glpk.h. The test is built against the uninstalled source, not against the installed package. If we want to build it against the installed package, we need to get the flags by looking at coinutil.pc, which would then pull in coinglpk.pc and get the right includes. We should then just have a hand-coded simple Makefile.in, as we have in some examples. Thinking about it, it would actually make a lot of sense to do this. Currently, we are testing against installed versions of dependencies, but against an uninstalled version of the package itself. But I'm going to leave it this way for now unless you tell me I'm wrong. We can fix this up for the whole stack after the merge.

Copy link
Member

Choose a reason for hiding this comment

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

Nice that the test had confirmed that glpk.h should not be included by the headers of CoinUtils.
If you need the Glpk header also for building the test, then you should extend the macro in configure.ac that checks for CoinUtils to something like

AC_COIN_CHK_PKG(Glpk,[CoinUtilsLib CoinUtilsTest],[coinglpk])

and then use COINUTILSTEST_CFLAGS.

The test is somewhere between the CoinUtils lib itself and building against an installed CoinUtils. It doesn't belong to the CoinUtils lib, so the COINUTILSLIB_* variables shouldn't be used. Therefore, it can test CoinUtils from the users point of view. But it also, intentionally, does not test against an installed version of CoinUtils (like the examples do). One first tests, then installs.

libtool makes sure that it is sufficient to unitTest_LDADD = ../src/libCoinUtils.la. Dependencies of CoinUtils should be stored in the .la file and are added to linker call for the unitTest binary by libtool.

I don't see benefit in manually coding a Makefile.in as for the examples. You cannot use the coinutils.pc anyway, since that is for the installed version of the project, but you want to test against an uninstalled one.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, yes, I was trying to think of how to use the configure script itself to build the flags for the unit test, but the approach you just suggested didn't occur to me. You're right, that is easier and also accomplishes the same thing.

The advantage I could see of manually coding a Makefile.in is that it is the closest you can get to using the installed library that way an actual user would. That is the reason why the examples are done that way. I could easily imagine cases where there would be an error turned up by running the unit test this way that would not be turned up otherwise.
But this means installing the library before it's tested and I guess that would defeat the purpose in a way. If the unit test fails after the library is already installed, then it should be manually uninstalled, which is a pain. Running one of the examples is fine for testing whether things got installed correctly post-install.

So I will follow your suggestion. But we should also just make it so that all the Glpk stuff is encapsulated in CoinUtils and we don't need to include glpk.h.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, thinking about this a bit more, I'm not so sure. In general, information about secondary dependencies should come through the primary dependency, i.e., if I want to know how to link to CoinUtils, I should query coinutils.pc, which should tell me what else I might need to know, like what other include directories I need to link against it and what additional libraries I might need. Glpk is not a direct dependency of the unit test in this case, its only a direct dependency of CoinUtils. So it feels a bit wrong that it would be listed as a direct dependency in configure.ac. All we're doing is precisely mirroring the flags that the library itself needs. So doesn't it make sense to make it clear that this is what's going on?

Copy link
Member

Choose a reason for hiding this comment

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

The problem (probably not too serious, though), is that precisely mirroring the flags that the library itself needs is probably not want you want for the test. The test should be build against the library and not like the library itself.
As said in the beginning, with COINUTILSLIB_CFLAGS you also get -DCOINUTILS_BUILD, so when the test is build, the full config.h is included instead of only config_coinutils.h. Thus, the test does not capture whether config_coinutils.h is incomplete or broken.
Having a second version of COINUTILSLIB_CFLAGS, i.e., COINUTILSTEST_CFLAGS would resolve this. The idea of COINUTILSLIB_CFLAGS was to collect the flags to build the lib, not really to build the test. The test is a separate target and if that needs more than the CoinUtils headers itself, there would be a sense to have separate COINUTILSTEST_* vars.

It seems that hiding the Glpk dependency from the CoinUtils user (in the sense that it doesn't expose the glpk cflags) is indeed not what Requires.private can achieve. :( There is some long text about this at https://gitlab.freedesktop.org/pkg-config/pkg-config/-/issues/7. I didn't read it all, but there seems to be a third thing now, Requires.internal, which we do not make use of. My pkg-config has a flag --internal-cflags with meaning "do not filter 'internal' cflags from output".

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, messy. But anyway, yeah, we don't really want -DCOINUTILS_BUILD. We essentially want the flags that would be returned by pkg-config --cflags coinutils, but I guess we can't actually get those except by either the method you suggest or the method I suggest, both of which actually use pkg-config to get the flags.

The description of the pkg-config bug you linked to seems to be based on a flawed understanding of -I flags. He claims that including -I/usr/include/libxml2 means that he is required to install the headers for libxmpl2, but this isn't the case. Adding more directories to the search path doesn't "require" anything. The critical issue is whether headers of one library include headers of another (in which case you really do need to have them installed) when they really don't need to, but this has nothing to do with pkg-config. This is is an issue for the package itself and is separate from whether pkg-config filters out the directories where those headers would live whenever they're not actually needed. I don't pkg-config's behavior can break any builds or cause issues other than very long and unwieldy lists of flags.

The issue we're talking about here is basically that same one. glpk.h is included in CoinMpsIO.hpp, which means that headers for glpk need to be installed to link to CoinUtils. It would be possible to change this by instead including glpk.h only in CoinMpsIO.cpp and just doing a typedef in CoinMpsIO.hpp, as you suggested. There are some advantages to that and I guess we should do it, but realistically, I don't think it's really possible to have the Glpk library installed and not have glpk.h installed as well, so I can't see a case where the current setup would actually cause a problem. And because of the behavior of pkg-config, the include directory would be included in the Cflags either way. This is not really an issue as far as I can see. But I guess we should clean this up.

Copy link
Member

Choose a reason for hiding this comment

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

The issue we're talking about here is basically that same one. glpk.h is included in CoinMpsIO.hpp, which means that headers for glpk need to be installed to link to CoinUtils. It would be possible to change this by instead including glpk.h only in CoinMpsIO.cpp and just doing a typedef in CoinMpsIO.hpp, as you suggested. There are some advantages to that and I guess we should do it, but realistically, I don't think it's really possible to have the Glpk library installed and not have glpk.h installed as well, so I can't see a case where the current setup would actually cause a problem. And because of the behavior of pkg-config, the include directory would be included in the Cflags either way. This is not really an issue as far as I can see. But I guess we should clean this up.

For package managers, headers are usually only part of the -devel version of a package. So libcoinutils-devel would require libglpk-devel and not only libglpk if one leaves glpk.h in the installed CoinUtils headers.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, yeah, I realized that, I guess I was assuming that the headers were not actually split out for smaller packages. but sure, makes sense. I fixed now in cb61db6.

@tkralphs tkralphs merged commit 9a2fd8a into master Jan 2, 2022
@svigerske svigerske deleted the refactor branch December 7, 2024 15:52
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.

5 participants