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

RFC: Use instances instead of types when specifying options #74

Merged
merged 3 commits into from
Oct 9, 2015

Conversation

tomasaschan
Copy link
Contributor

This opens up for actually keeping data in the options, even if that data is not of bits types (and thus not possible to include as a type parameter), for example a boundary condition Tangent(k) with an interpolation object that has eltype(itp) <: Array.

Additional benefits:

  • Less confusion on correct call signatures - now you always pass an instance
  • Less to type (you never have to type Tuple again, not to mention curly braces...)
  • "Automatically" solves the same problem as Allow manual override of return type for fillvalue #72 intended to do, but in an (in my opinion) more elegant way: now, FillValue(2) will convert its argument to eltype(itp) (keeping getindex type-stable), while TypedFillValue(2) will not (rendering getindex type-unstable, but allowing for extrap values that cannot be converted to eltype(itp)). I'll probably add convenience methods Fill(2) and Fill(T, 2) to do the same thing if this PR is merged.

Drawbacks:

  • Leads to "parenthesis-hell" reminiscent of LISP (take a look at interpolate(A, (BSpline(Linear()), BSpline(Quadratic(Flat()))), OnGrid()) or extrapolate(itp, (Flat(), Linear()), (Flat(), Periodic()))...)
  • ...?

Still need to

  • Update documentation

but I would very much like some input on whether this is a good idea or not, since it's very breaking for the entire package.

@timholy
Copy link
Member

timholy commented Sep 29, 2015

Without needing to pass data and having no other good way to do it, I confess to not being a fan. But let's see what others say.

@tomasaschan
Copy link
Contributor Author

My main reason for this is that it makes implementation of a Tangent BC (which was explicitly requested, see #5 (comment)) quite simple and elegant - we could probably even re-wire the Flat code we have now to include the inhomogeneity in the set of equations, and just add Flat() = Tangent(0,0) and be done with it.

I also like this version, and its extensibility, of the Fill() extrapolation much better than #72, but I guess that too is a matter of taste.

@tomasaschan
Copy link
Contributor Author

One way to get out of parenthesis hell would be to add and export a bunch of singleton constants, that hold instance values of each. That is,

module Interpolations

# ...

export
# ...
    flat,
    constant,
    linear,
    quadratic,
    periodic,
    reflect
# etc...

const constant = Constant()
const flat = Flat()
const linear = Linear()
# etc

allowing for usage like itp = interpolate(A, BSpline(linear)), but I fear this is just hiding from the user what's really going on, when there's really no need to.

What are your main pain points?

@timholy
Copy link
Member

timholy commented Sep 29, 2015

I'm willing to go with the change. It's really mostly the parens hell I'm unhappy about. It would be worth thinking about whether there's a deprecation strategy, though.

@tomasaschan
Copy link
Contributor Author

Actually, since the interfaces aren't really in conflict, we could just keep both, and make the old one forward to the new (to keep just one internal implementation). I'd still want the docs to use instances, but there really isn't much of a difference in code complexity otherwise (except that we'll have to do some manual handling to go from Tuple{...} to (...), since we can't just tack () on the end to construct the tuples).

If we don't go that way, and want a deprecation strategy, I think we could leverage the deprecation helpers from Base and do something like this:

  1. Add back the old methods, with deprecation warnings. (I couldn't find a clean way to do it with @deprecate, but manual depwarns should be good enough.)
  2. Merge this PR and tag a new version - hopefully before Julia 0.4.0 is out.
  3. Let the old interface sit, with deprecation warnings, for at least a month or two, regardless of tagging new versions for adding other stuff.
  4. Remove old cruft :)

@timholy
Copy link
Member

timholy commented Sep 30, 2015

Sounds good! I guess we could leave both in place a while to see if it causes trouble, and decide later about the deprecation strategy.

This opens up for actually keeping data in the options, even if that
data is not of bits types (and thus not possible to include as a type
parameter), for example a boundar condition Tangent(k) with an interpolation
object that has eltype(itp) <: Array.
@tomasaschan
Copy link
Contributor Author

@timholy I've re-added the type-based interface. Do you think this makes us good to merge this?

@tomasaschan
Copy link
Contributor Author

As this isn't breaking anymore, I'll take your silence as a "yes". :)

tomasaschan pushed a commit that referenced this pull request Oct 9, 2015
RFC: Use instances instead of types when specifying options
@tomasaschan tomasaschan merged commit aefa731 into master Oct 9, 2015
@tomasaschan tomasaschan deleted the instances branch October 9, 2015 13:40
@timholy
Copy link
Member

timholy commented Oct 9, 2015

Good call. Been traveling and too busy to reply.

@timholy timholy mentioned this pull request Aug 14, 2018
4 tasks
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.

2 participants