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

MAINT: Clean up optimize.zeros C solvers interfaces/code. #9465

Merged
merged 1 commit into from Mar 28, 2019

Conversation

@pvanmulbregt
Copy link
Contributor

pvanmulbregt commented Nov 10, 2018

Split data being passed to zeros C solvers into const/modifiable parts.
Refactored to remove an unneeded jmp_buf object.
Added volatile qualifier to a object used after a longjmp() return.

In optimize.zeros, the C solvers were being passed a scipy_zeros_parameters
struct disguised as a default_parameters struct. The initial elements of the
scipy_zeros_parameters contained the same elements as the default_parameters,
and pointers to default_parameters were being upcast to scipy_zeros_parameters.
However these two structs were not linked either in code or documentation.
This change separates the data needed to evaluate the function from the
statistics collected during the solving of the equation. This also makes for
easier initialization and cleaner separation of input and output data.
Ensure the solver_stats->error_num is set for all return paths.
The jmp_buf does not need to be copied if it is constructed in place.
A PyTuple is being created and then destroyed in both branches after a
setjmp() return, hence needs a volatile qualifier.

…le parts.

Refactored to remove an unneeded jmp_buf object.
Added volatile qualifier to a object used after a longjmp() return.

In optimize.zeros, the C solvers were being passed a scipy_zeros_parameters
struct disguised as a default_parameters struct.  The initial elements of the
scipy_zeros_parameters contained the same elements as the default_parameters,
and pointers to default_parameters were being upcast to scipy_zeros_parameters.
However these two structs were not linked either in code or documentation.
This change separates the data needed to evaluate the function from the
statistics collected during the solving of the equation.  This also makes for
easier initialization and cleaner separation of input and output data.
Ensure the solver_stats->error_num is set for all return paths.
The jmp_buf does not need to be copied if it is constructed in place.
A PyTuple is being created and then destroyed in both branches after a
setjmp() return, hence needs a volatile qualifier.
@pvanmulbregt

This comment has been minimized.

Copy link
Contributor Author

pvanmulbregt commented Jan 30, 2019

I'd like to see this change to the 1d root solvers internal C API in place before this C API is exposed as proposed in #8431. This modified API is cleaner/safer. Merging it prior to #8431 avoids consideration of changing a public API right after it is first exposed.

Copy link
Contributor

mikofski left a comment

I think this is a really valuable addition. Thanks so much for doing this work.

@mikofski

This comment has been minimized.

Copy link
Contributor

mikofski commented Jan 30, 2019

closes #9189 indirectly

@pvanmulbregt

This comment has been minimized.

Copy link
Contributor Author

pvanmulbregt commented Mar 28, 2019

If #8431 and #9465 are both to be merged, #9465 should be merged first as it cleans up the internal API before it is made public, hence a decision is needed.

@rgommers rgommers added this to the 1.3.0 milestone Mar 28, 2019
@rgommers

This comment has been minimized.

Copy link
Member

rgommers commented Mar 28, 2019

Sounds like a good idea, and since @mikofski is also happy with it I don't see a reason not to merge this. I'll have closer look now.

@rgommers rgommers merged commit e4f5a25 into scipy:master Mar 28, 2019
4 checks passed
4 checks passed
ci/circleci: build_docs Your tests passed on CircleCI!
Details
ci/circleci: pypy3 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rgommers

This comment has been minimized.

Copy link
Member

rgommers commented Mar 28, 2019

This looks understandable and clean, so merging. Thanks @pvanmulbregt! And thanks @mikofski for reviewing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.