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

Get function call from Interpolations.jl working and submitted as a PR #26

Closed
4 tasks done
jlperla opened this issue May 25, 2018 · 12 comments
Closed
4 tasks done

Comments

@jlperla
Copy link
Contributor

jlperla commented May 25, 2018

See JuliaMath/Interpolations.jl#198 (comment) for a starting point. I want to add support (not replace) for calling the Interpolations.jl library with () rather than [].

We should have this as a full-fledged PR so that someone can just look at this, approve, etc. You can discuss any details on here that are unclear.

@chiyahn
Copy link
Collaborator

chiyahn commented Jun 15, 2018

I have completed implementing method calls with corresponding new unit tests passed.

One drawback of my approach was that I had to define this for all derived types being used for interpolation, including GriddedInterpolation and BSplineInterpolation, which makes the code a bit ugly. An intuitive way to deal with this would be to define it in AbstractInterpolation that all these types are derived from instead, but it seems like defining a concrete method in an abstract type is not supported in the most recent version of Julia. There have been some relevant discussion in JuliaLang/julia#14919.

There are other work arounds, like using macros or redefining AbstractInterpolation as a concrete type, which all I have tried, but they all suffer from either code instability or need of significant changes in the code. At this moment it seems like manually defining function calls for all concrete subtypes would be the most reasonable choice, but I am afraid this would not be an efficient way to handle it, as all subsequent contributors who want to add new concrete types of interpolation (if there are) have to implement a function call feature manually.

@jlperla
Copy link
Contributor Author

jlperla commented Jun 15, 2018

Sounds great. Yes, sounds like Chris said you would need to implement it for all derived types. A pain, but so be it. Did you add document changes as well in the checklist? If we want to try to get them to take this as a PR, we would need that to be complete. After that, why don't you create a PR and submit it to the library. I will then ask a few people to review it.

@chiyahn
Copy link
Collaborator

chiyahn commented Jun 19, 2018

No problem! Pull request has been made here: JuliaMath/Interpolations.jl#206

@jlperla
Copy link
Contributor Author

jlperla commented Jun 19, 2018

Spectacular!

Are you able to request code reviews on your PR?

@chiyahn
Copy link
Collaborator

chiyahn commented Jun 19, 2018

I believe I have to wait until the repo owners assign a reviewer; is there a way of asking someone to do so? 😮

@jlperla
Copy link
Contributor Author

jlperla commented Jun 19, 2018

I am going to try to see if I can get people to look at it sooner rather than later. Do you want to take a break in the meantime, or are you ready for more issues!

@chiyahn
Copy link
Collaborator

chiyahn commented Jun 20, 2018

I am definitely ready for more issues 😎

@jlperla
Copy link
Contributor Author

jlperla commented Jul 10, 2018

OK, I think this is almost ready to be closed out. After the arguments on your PR about the best strategy, there was agreement on comment JuliaMath/Interpolations.jl#206 (comment)

Basically, on your PR:

After that, tell me and I will ask Spencer to merge it.

@chiyahn
Copy link
Collaborator

chiyahn commented Jul 16, 2018

Task complete! Note that I have created separate unit tests for examples included in README.md in test/readme-examples.jl with a change in test/runtests.jl accordingly.

Should I notify maintainers of the change in PR?

@jlperla
Copy link
Contributor Author

jlperla commented Jul 16, 2018

I think the last thing that the maintainer seemed to want was an Issue added which gives a sense roughly of what would be needed for a full deprecation of []. After you create that issue. I would ping sglyon on that PR (pointing to the issue) and say that you think you have done everything they have asked in that, that you would like them to do a final check and merge?

@chiyahn
Copy link
Collaborator

chiyahn commented Jul 17, 2018

No problem! I made a new issue here: JuliaMath/Interpolations.jl#212

@jlperla
Copy link
Contributor Author

jlperla commented Jul 17, 2018

OK, let me ping them then. I think we can close this issue!

@jlperla jlperla closed this as completed Jul 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants