Skip to content

RFC: Expose xnew in C_wrapper, maintaining compat - #322

Closed
odow wants to merge 1 commit into
masterfrom
od/expose-xnew
Closed

RFC: Expose xnew in C_wrapper, maintaining compat#322
odow wants to merge 1 commit into
masterfrom
od/expose-xnew

Conversation

@odow

@odow odow commented Jul 18, 2022

Copy link
Copy Markdown
Member

Replacement for #313, just so I can try it out. I don't know if we should merge this. The alternative is for the user to write a closure, which isn't that much work.

cc @sohailrreddy

@odow odow changed the title Expose xnew in C_wrapper, maintaining compat RFC: Expose xnew in C_wrapper, maintaining compat Jul 18, 2022
@codecov

codecov Bot commented Jul 18, 2022

Copy link
Copy Markdown

Codecov Report

Merging #322 (3ea2f8f) into master (e5f978c) will increase coverage by 0.27%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #322      +/-   ##
==========================================
+ Coverage   87.80%   88.07%   +0.27%     
==========================================
  Files           3        3              
  Lines         697      713      +16     
==========================================
+ Hits          612      628      +16     
  Misses         85       85              
Impacted Files Coverage Δ
src/C_wrapper.jl 88.54% <100.00%> (+1.59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5f978c...3ea2f8f. Read the comment docs.

@odow

odow commented Oct 20, 2022

Copy link
Copy Markdown
Member Author

@sohailrreddy are you still interested in this? If not, I will close.

From #313 (comment):

I'd like to see a practical application where this makes a difference.

@odow

odow commented Nov 30, 2022

Copy link
Copy Markdown
Member Author

Closing as stale. I understand the reasoning for this, so if anyone has a benchmark where this makes a difference in practice, please post a comment and we can re-open the PR.

@odow odow closed this Nov 30, 2022
@odow
odow deleted the od/expose-xnew branch November 30, 2022 21:28
@DaniFojo

DaniFojo commented Dec 19, 2025

Copy link
Copy Markdown

Hello. Exposing xnew could really be useful for us.

The reason is that our methods to compute objective, grad and constraint values all need to compute some shared data first, and then use that shared data. If xnew is exposed we can know if we need to recompute that shared data. The memoization closure won't work for us.

For example, if eval_f was just called with a given x, if eval_grad_f is called with the same x we can skip this computation.

Right now we are using our own fork of the package to do this, but I think it could be update this PR and merge it. I can do the PR if needed.

@odow

odow commented Dec 19, 2025

Copy link
Copy Markdown
Member Author

I don't see why this can't be achieved with a closure? Please open a PR with your changes if you would like to discuss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants