Skip to content

direct: diff from SciPy #615

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lucascolley
Copy link

@lucascolley lucascolley commented May 4, 2025

This draft PR shows the diff resulting from replacing the files of the direct algorithm here with what we have in SciPy. It looks like there is a mixture of (many) small formatting things, and (some) substantive changes.

Would there be interest in merging these sources into 'one true source'? This seems desirable, to de-duplicate work, ensuring any fixes made here end up in SciPy, and any contributions to SciPy end up here. We could set up a more formal vendoring situation to facilitate this once the sources are matched.

It may be that some of the changes here seem problematic, and we would rather change things in SciPy to match what is already here. That is also on the table, let me know what you think! In particular, it looks like we would want to split the PyObject parts out into separate bindings, either here on in SciPy.

Cross reference scipy/scipy#21232

@lucascolley
Copy link
Author

if there is interest in proceeding here, perhaps we could merge all of the trivial formatting changes, then take a closer look at how to deal with the reduced diff

Comment on lines +93 to +102
#define MAXMEMORY 1073741824
integer MAXFUNC = *maxf <= 0 ? 101000 : (*maxf + 1000 + *maxf / 2);
integer fixed_memory_dim = ((*n) * (sizeof(doublereal) + sizeof(integer)) +
(sizeof(doublereal) * 2 + sizeof(integer)));
MAXFUNC = MAXFUNC * fixed_memory_dim > MAXMEMORY ? MAXMEMORY/fixed_memory_dim : MAXFUNC;
c__ = (doublereal *) malloc(sizeof(doublereal) * (MAXFUNC * (*n)));
if (!(c__)) {
*ierror = -100;
goto cleanup;
}

Choose a reason for hiding this comment

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

This part was added as the algorithm uses absurd amounts of memory for large number of function evaluations. The MY_ALLOC macro did not help, we got a segmentation fault. Could this be of interest to nlopt @stevengj ?

@dschmitz89
Copy link

A lot of these change change functions from void to PyObject. This makes sense for scipy but probably not so much for a C library like nlopt. See above for one finding we had related to memory issues. I guess a better solution long term would be to wrap nlopt's reimplementation of DIRECT which uses trees instead of arrays to reduce memory usage.

@lucascolley
Copy link
Author

A lot of these change change functions from void to PyObject. This makes sense for scipy but probably not so much for a C library like nlopt

Is there some way to keep the C library returning void, and add a wrapper layer to PyObject on top that could live in SciPy?

@dschmitz89
Copy link

dschmitz89 commented May 8, 2025

CC @czgdp1807 as he knows most about the wrapping magic. A word of caution though: getting DIRECT into scipy was a substantial amount of work and we might just let it rest in its current state as I do not see open issues related to it (might also just be little adoption of it compared to SHGO and friends on the scipy side though).

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.

2 participants