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

Update kopt #214

Closed
wants to merge 22 commits into from
Closed

Update kopt #214

wants to merge 22 commits into from

Conversation

nbelakovski
Copy link
Contributor

Attached are redone profiles. There are slight improvements.

cobyla_different_options.pdf
cobyla_nondefault_options.pdf
cobyla_default_options.pdf

nbelakovski and others added 22 commits January 7, 2025 14:26
SciPy complained about "expression result unused" for the extra '(',
which makes sense.

It also complained about `rc` being unused. This is slightly annoying
because we put the return value of `prima_minimize` in `result.status`.
I decided it made sense to return info by itself if there was an error
in either `init_result` or `check_problem`, and then just return DFT by
default.
- gcc 13.3.0 seems to have an issue with math.h on macOS. It spews a
  bunch of errors about the API_DEPRECATED macro not being used
  correctly, which is not an issue on our side. gcc 13.2.0 does not
  seem to have this issue, but since we cannot select gcc with such
  granularity we downgrade to gcc 12 and hope this fixes the issue for
  now.

- numpy 2.0.0 has come out which causes runtime issues. Fortunately the
  fix is easy, just build with pybind11 >=2.12.0. However this is still
  an issue for the pdfo compatibility test since pdfo does not support
  numpy 2.0.0, and sometimes the tests install 2.0.0 because it happens
  to be in the pip cache. So we upgrade pybind11 (the new version is
  compatible with 2.0 and 1.0 numpy) and skip the pdfo test if we
  happened to pick up numpy 2.

- macOS builds for building Python wheels started to complain when running
  the "delocate" step that libquadmath, libstdc++, and libgfortran have
  minimum targets of 11.0, whereas the default was 10.9, hence we added
  the relevant env variable for CIBW.

- Notes have been made in build_python.yml for failures related to
  Ubuntu 24.04.

- A couple tests were failing on macOS 14. I tried to modify rhobeg and
  rhoend, but this ended up turning into a game of whack-a-mole, where a
  rhobeg/rhoend that worked on one architecture would fail on another.
  Ultimaately the most reliable solution I found was to modify the
  starting point to be close to the optimal point.
This is in support of efforts to integrate PRIMA with SciPy. They
provide wheels for musl, and in musl you cannot have nested functions in
Fortran (at least, not without gcc 14, which is cutting edge, and
therefore it's not practical for scipy to switch to it).
This is in support of adding PRIMA to SciPy. The nested function does
not work in musl on alpine. The reason is that alpine does not support
executable stacks. This internal function does work with gcc 14 since it
introduces -ftrampoline-impl=heap (as opposed to stack), but gcc 14 is
too cutting edge for scipy.

The module level variables are unfortunately not thread safe. Depending
on demand we could bring back the nested function under a flag, but I
think it's prudent to wait for demand for such a thing before
implementing it.
Also remove macos 11 since it can never seem to find runners.
This is mainly for SciPy since they would like to trim the binary size
by not including the other solvers. By default all solvers will be
built, so the way to use this is to define DONT_BUILD_BOBYQA or whatever
algorithm you don't want.

The use of the double negative is unfortunate, but the alternative would
be to require users to explicitly build the algorithms they want, which
seems burdensome.
calcfc_chebyqud and calcfc_hexagon in Python return values close to what is returned by Fortran,
but not exactly the same and the difference is outside of machine precision.

For chebyquad the fortran version iterates 387 times within cobylb, whereas
the Python version iterates 564 times. The results are as follows

Fortran:
x(1) = 0.06687201625695266
x(2) = 0.28872054433564054
x(3) = 0.36668681233017669
x(4) = 0.63330363432938008
x(5) = 0.71126615229302259
x(6) = 0.93312277720964698
f = 3.9135366518694255e-10

Python:
x[0] = 0.06688196439284098
x[1] = 0.2887610493288529
x[2] = 0.36668226015206873
x[3] = 0.6333320849097046
x[4] = 0.7112583722245663
x[5] = 0.9331254078707577
f = 3.8259601181730434e-10

The first position in x differs by 9.948135888e-6, so it's further away than machine precision, but
it's possible that the difference is the result of accumulation of machine precision errors.
After much effort, the Python cobyla code has been reconciled with the
upstream fortran code. At a basic level it seems to work, and for
calcfc_chebyquad it seems to work fine, but there's still quite a bit of
room for improvement on calcfc_hexagon.
Now cobyla agrees on the contrained problem. The issue was changes in
the fortran example that had not made it to the python version.

Next step is optiprofiler.
Some testing revealed a bug with selection of kopt. Using the mask
caused the indices to be mixed up, so we need to use numpy masking
facilities.

Earlier I had tried using np.where, but this also created a bug.
Fortran's minloc will not consider any indices covered by the mask,
whereas the "where array == min(array[mask])" construct will allow
indices covered by the mask. So other instances of np.where were fixed.

In investigating the bug it was discovered that findpole differed from
the current latest Fortran implementation, likely because it was
initially incorrectly implemented from the update.f90 in the benchmark
folder.
@nbelakovski
Copy link
Contributor Author

Accidentally opened against main prima repo instead of my own, apologies.

@nbelakovski nbelakovski closed this Jan 9, 2025
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

check-spelling found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

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.

1 participant