-
Notifications
You must be signed in to change notification settings - Fork 109
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
Conversation
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. |
My main reason for this is that it makes implementation of a I also like this version, and its extensibility, of the |
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,
allowing for usage like What are your main pain points? |
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. |
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 If we don't go that way, and want a deprecation strategy, I think we could leverage the deprecation helpers from
|
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. |
21a0233
to
7d86a7a
Compare
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.
@timholy I've re-added the type-based interface. Do you think this makes us good to merge this? |
As this isn't breaking anymore, I'll take your silence as a "yes". :) |
RFC: Use instances instead of types when specifying options
Good call. Been traveling and too busy to reply. |
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 haseltype(itp) <: Array
.Additional benefits:
Tuple
again, not to mention curly braces...)FillValue(2)
will convert its argument toeltype(itp)
(keepinggetindex
type-stable), whileTypedFillValue(2)
will not (renderinggetindex
type-unstable, but allowing for extrap values that cannot be converted toeltype(itp)
). I'll probably add convenience methodsFill(2)
andFill(T, 2)
to do the same thing if this PR is merged.Drawbacks:
interpolate(A, (BSpline(Linear()), BSpline(Quadratic(Flat()))), OnGrid())
orextrapolate(itp, (Flat(), Linear()), (Flat(), Periodic()))
...)Still need to
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.