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

Inequality direction in wrapper functions #148

Closed
hwborchers opened this issue Nov 12, 2023 · 5 comments
Closed

Inequality direction in wrapper functions #148

hwborchers opened this issue Nov 12, 2023 · 5 comments

Comments

@hwborchers
Copy link

hwborchers commented Nov 12, 2023

I am a contributor to the {nloptr} package. I contributed the small 'wrapper' functions --like 'lbfgs', 'auglag', or 'slsqp', etc. - that hopefully made it easier for casual users of the package to call basic routines without the need to understand the API of the main function 'nloptr()'.

I have a concern or request:

For instance, in the 'slsqp' function I interpreted the inequality constraints as hin(x) >= 0 while Jelmer Ypma thought hin(x) <= 0 would be more appropriate in the context of the package. Therefore, he added a message

"For consistency with the rest of the package the inequality sign
 may be switched from >= to <= in a future nloptr version."

This is correct but becomes confusing or annoying when calling this function one or several times. Can we change this behavior, either by removing the message or by reversing the inequality sign?

Reversing the sign will, of course, change the function behavior and will probably lead to "dependency problems". I have just gone through this process, see https://stat.ethz.ch/pipermail/r-help/2023-November/478539.html, and unfortunately, the added message is not an official "deprecated" warning.

I don't mind if you prefer to leave things as they are. In this case, to not confuse the users too much it would be appropriate to remove the message. (I have not checked whether similar messages appear in other wrappers.) Perhaps the direction of the inequality constraint could be emphasized more clearly on the corresponding help pages.

Hans Werner

@aadler
Copy link
Contributor

aadler commented Nov 12, 2023

Hello, @hwborchers. In theory, I would like $\leq$ to be the default for consistency with the rest of the basic definitions, but I would suggest that if we follow that approach it should be a three-step process with suitable time (at least months) between steps:

  1. Deprecate it now whilst building in an "old behavior" option which is the default. If we follow Semantic Versioning we would bump the MINOR version (added functionality but no API change).
  2. Switch the default option to the NEW behavior. If we follow Semantic Versioning we would bump the MAJOR version (breaking API change).
  3. Defunct the old behavior. If we follow Semantic Versioning we would bump the MINOR version (added functionality but no API change).

In the interim, perhaps the simplest thing is to update the help to reference SuppressMessages for those who find it annoying?

@aadler
Copy link
Contributor

aadler commented Jun 4, 2024

I am going to work on this in the devel branch of my fork.

@astamm
Copy link
Owner

astamm commented Jun 15, 2024

I agree with @aadler. In the long run, we should implement all inequality constraints with the same $\le$ sign. And, yes, we should clarify the help in the meantime and implement a deprecation strategy.

@aadler how far did you manage to go on your fork on this issue?

For the deprecation process, we could implement that with lifecycle.

@astamm
Copy link
Owner

astamm commented Jun 15, 2024

As I am planning a release to CRAN next week, we also need to decide if step 1 of the deprecation process should appear in this release.

@astamm
Copy link
Owner

astamm commented Jun 15, 2024

Should be fixed in #157 thanks to @aadler .

@astamm astamm closed this as completed Jun 15, 2024
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Nov 6, 2024
# nloptr 2.1.1

This is a patch release to work around a bug in the CRAN checks. Specifically,
one of the unit tests for the `isres()` algorithm was failing on some CRAN
builds because convergence is stochastic with slightly different results even
with the same fixed seed prior to calling the function.

# nloptr 2.1.0
This release deprecates the default behavior of the inequality equations in any
wrapper function which uses them. Currently, they are calibrated to be >= 0.
This version allows for the equations to be consistent with the main `nloptr`
function, which requires <= 0. In a future release, the default behavior will
switch to assuming the calibration is <= 0, and eventually, the >= 0 behavior
will be removed. It also includes a large number of safety and efficiency
changes, and an expansion of the unit tests to 100% coverage for all files but
one. The major changes include:

* Reversed the direction of the inequality equations `hin` and `hinjac` in the
wrapper functions which use them, bringing them into compliance with the main
`nloptr` call. This addresses
[Issue #148](astamm/nloptr#148);
* Cleaned the Hock-Schittkowski problem no. 100, Hartmann 6-dimensional, and
Powell exponential examples. This addresses
[Issue #152](astamm/nloptr#152) and
[Issue #156](astamm/nloptr#156);
* Updated roxygen version;
* Updated maintainer email;
* Deal with NA returns from `parallel::detectCores()` (contributed by @jeroen in
PR #150);
* Setup rhub v2 checks;
* Update cmake installation instructions on Mac with brew (#146);
* Allow use of equality constraints with COBYLA (#135);
* Replaced the unit testing framework of `testthat` with `tinytest` (See
[Issue #136](astamm/nloptr#136));
* Brought coverage of `is.nloptr` to 100%. The only file not completely covered
by unit tests is `nloptr.c`. The uncovered calls are error messages which get
trapped by tests in R before the call gets to C;
* Linted package for code correctness and consistency;
* Updated vignette, DESCRIPTION, and NEWS;
* Updated package website to use bootstrap 5;
* Expanded unit tests: coverage now over 97% with no file below 90%;
* Removed forcing `C++11`;
* Added safety checks to C code;
* Added many safety and efficiency enhancements to R code;
* Most R code style made self-consistent;
* Updated documentation and messages for accuracy and mathematical formatting
* Updated Github actions;
* Some bugfixes (e.g. in `isres` or the warning in `nl.grad`).
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

3 participants