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

Enable support for cython 3 #227

Merged
merged 2 commits into from
Nov 5, 2023
Merged

Conversation

chrhansk
Copy link
Contributor

This is an attempt to fix #211:

  • All callbacks are now marked noexpect with exceptions being handled (stored) internally
  • A large try / expect block is used in the callbacks, where code outside of the blocks should not throw (only stack allocations)
  • The large Jacobian / Hessian callbacks have been split up into value / structure callbacks
  • Cython restriction to <3 was removed

@chrhansk
Copy link
Contributor Author

Edit: Everything is cast to Problem rather than object in this patch, so the callbacks have access to all (not just public) cdef properties, which could now be made private (by removing the public) without problems.

@moorepants
Copy link
Collaborator

This will need to run with Cython 0.29.* and 3+. Does it?

@chrhansk
Copy link
Contributor Author

It is supposed to and does on my system.

@musicinmybrain
Copy link
Contributor

musicinmybrain commented Nov 4, 2023

This will need to run with Cython 0.29.* and 3+. Does it?

This works for me (builds and tests pass, except any pre-existing issues like #237 and #238) on Fedora Linux Rawhide as well. (I did also test it with Cython 0.29.35 for completeness.) I’ll probably go ahead and apply this as a downstream patch in Rawhide, since this is one of a handful of packages that still needs Cython 0.29, and we’d like to stop maintaining that compat package sooner rather than later.

_x = np.zeros((n,), dtype=DTYPEd)

for i in range(n):
_x[i] = x[i]
Copy link
Collaborator

Choose a reason for hiding this comment

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

How will these new lines raise a CyIpoptEvalutionError? It isn't clear why you've moved them in this try/except.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, the bare except clause was hidden by github.

@moorepants
Copy link
Collaborator

I've gone through this and I don't see any issues with the changes. Thanks.

@moorepants moorepants merged commit 9b9d98d into mechmotum:master Nov 5, 2023
34 checks passed
@chrhansk
Copy link
Contributor Author

chrhansk commented Nov 6, 2023

Thank you for the merge.

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.

Does not build with Cython 3
3 participants