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

BREAKING: Unexport gradient and friends #225

Closed
wants to merge 3 commits into from

Conversation

tomasaschan
Copy link
Contributor

@tomasaschan tomasaschan commented Aug 13, 2018

This is another take on solving the same problems as #222, but instead of exporting a new function gradient, I have removed it (and its friends) from the list of exported functions.

By qualifying all usages in the test suite as Interpolations.gradient, they now pass on 1.0! 🎉

However, I'd like to figure out a way to make this transition a little kinder to the users. Is there a way to deprecate usage like

using Interpolations
itp = ...
gradient(itp, ...)

while still allowing

using Interpolations
itp = ...
Interpolations.gradient(itp, ...)

?

If not, maybe we just have to accept that this is part of a somewhat painful transition to 1.0.

Edit: I've incorporated #223 here too, because making it work on Julia 1.0 is the main motivation behind this PR. Doesn't matter to me if we merge #223 first and then this (which will effectively rebase that commit away) or close #223 and take the change from here.


Side note: It's not so weird to me after making these changes that we have problems with Hessians (se e.g. #122 and #181). There's not a single mention of hessian in the test suite...

@codecov-io
Copy link

codecov-io commented Aug 13, 2018

Codecov Report

Merging #225 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #225   +/-   ##
=======================================
  Coverage   81.73%   81.73%           
=======================================
  Files          27       27           
  Lines        1002     1002           
=======================================
  Hits          819      819           
  Misses        183      183
Impacted Files Coverage Δ
src/Interpolations.jl 90.32% <ø> (ø) ⬆️

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 6e48e07...03d6409. Read the comment docs.

@tomasaschan tomasaschan mentioned this pull request Aug 13, 2018
7 tasks
@tomasaschan tomasaschan requested a review from timholy August 13, 2018 12:35
@sglyon
Copy link
Member

sglyon commented Aug 13, 2018

I agree with the notion of Interpolations not "claiming" the function gradient (can't remember which issue I saw that comment in) and not exporting the function is a good way to accomplish that.

I think users have two options:

using Interpolations
itp = ...
Interpolations.gradient(itp, ...)

or

using Interpolations
using Interpolations.gradient
itp = ...
gradient(itp, ...)

As long as gradient has not been loaded/imported by another used package already.

If we are content with that outcome I think this PR is good to go!

@timholy
Copy link
Member

timholy commented Aug 13, 2018

If you're eager this can be merged, but I already have this on my local branch and lots of other work too.

If you do merge, I would argue against tagging 0.7/1.0 compatible just yet. I am getting very close (<1day?) to having a full rewrite done.

@tomasaschan
Copy link
Contributor Author

It doesn't matter much to me - I just reacted to the fact that quite a lot of people seemed eager to get something 1.0-compatible out there, judging by the number of recent PR:s trying to accomplish this. With this, there is a git hash people can check out if they need something now, but I see no reason not to wait for your larger batch of changes in general.

@timholy
Copy link
Member

timholy commented Sep 18, 2018

Superseded by #226

@timholy timholy closed this Sep 18, 2018
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.

5 participants