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

user supplied jacobian #46

Merged
merged 3 commits into from
May 15, 2017
Merged

Conversation

bjarthur
Copy link
Contributor

@bjarthur bjarthur commented Mar 9, 2017

this speeds up my particular use case by 3x.

not sure of the best way to expose the interface to curve_fit. for the time being i just left it unchanged.

@bjarthur bjarthur force-pushed the bja/jacobian branch 3 times, most recently from 9b34837 to eb20551 Compare March 9, 2017 15:14
@bjarthur
Copy link
Contributor Author

bjarthur commented Mar 9, 2017

tests are only failing b/c of the Rmath download is stalling.

@pkofod
Copy link
Member

pkofod commented Mar 9, 2017

Wonderful; finite differences can be really horrible sometimes. This will not only be a speed improvement, but also accuracy improvement in many cases.

edit: btw, please show the functionality in the README.md.

@bjarthur
Copy link
Contributor Author

bjarthur commented Mar 9, 2017

another alternative to finite differences is JuliaMath/Calculus.jl#94. i can work that up into a PR if you like. in my use case it was slower, but as you point out, likely more accurate.

any thoughts on hoisting this to the level of curve_fit? the case without weights is easy. not sure what to do with them.

@bjarthur
Copy link
Contributor Author

bjarthur commented Mar 9, 2017

updated README, and hoisted interface to curve_fit.

am i right in thinking that the Jacobian simply scales with the weights just like the model does?

@marcusps
Copy link
Contributor

marcusps commented Mar 9, 2017

@bjarthur Re: handling weights with the Jacobian, have a look at #45 . If that gets merged first, it should be easy to modify your code to handle the cases with vector and matrix weights.

@blakejohnson
Copy link
Contributor

@bjarthur I merged #45. Could you please fix up the merge conflicts here?

@bjarthur
Copy link
Contributor Author

bjarthur commented May 4, 2017

@marcusps could you please look this over re. the weights?

@blakejohnson conflicts fixed and tests have passed

@marcusps
Copy link
Contributor

marcusps commented May 4, 2017 via email

@pkofod
Copy link
Member

pkofod commented May 4, 2017

this speeds up my particular use case by 3x.

I see you added the new syntax to the README. Could you perhaps also add an example (the test problem for example) to show the performance increase? That would be great, as it makes it much easier for users to see exactly what they're supposed to do.

@bjarthur bjarthur force-pushed the bja/jacobian branch 2 times, most recently from 00c68ae to 3231c3f Compare May 4, 2017 19:47
@pkofod
Copy link
Member

pkofod commented May 4, 2017

Excellent! I have no further comments.

@bjarthur
Copy link
Contributor Author

bjarthur commented May 4, 2017

example added to README. i also took the liberty to fix a couple deprecation warnings, and to reformat the leading white space in one file.

@bjarthur
Copy link
Contributor Author

@marcusps is my handling of the weights here kosher? would be nice to merge this PR...

Copy link
Contributor

@marcusps marcusps left a comment

Choose a reason for hiding this comment

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

Looks good to me -- apologies for the delay.

@blakejohnson blakejohnson merged commit 1f9abe5 into JuliaNLSolvers:master May 15, 2017
@bjarthur bjarthur deleted the bja/jacobian branch May 15, 2017 17:21
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.

4 participants