[oneMKL] add Data Fitting domain to oneMKL #413
Conversation
source/elements/oneMKL/source/domains/data_fitting/interpolate.rst
Outdated
Show resolved
Hide resolved
source/elements/oneMKL/source/domains/data_fitting/interpolate.rst
Outdated
Show resolved
Hide resolved
| If the sites do not belong to interpolation interval ``[a, b]``, the library uses: | ||
|
|
||
| - interpolant :math:`I_0` coefficients computed for interval :math:`[x_0, x_1)` for the | ||
| computations at the sites to the left of ``a``. | ||
| - interpolant :math:`I_{n-2}` coefficients computed for interval | ||
| :math:`[x_{n-2}, x_{n-1})` for the computations at the sites to the right of ``b``. |
There was a problem hiding this comment.
You mean to say that you do allow for extrapolation, but it uses the interpolant function defined on the closest sub-interval in the interpolation interval :math:[a,b)?
There was a problem hiding this comment.
This seems different than your definition of site in the terms.rst
There was a problem hiding this comment.
You mean to say that you do allow for extrapolation, but it uses the interpolant function defined on the closest sub-interval in the interpolation interval :math:
[a,b)?
Correct.
This seems different than your definition of site in the terms.rst
Statements here are correct. Will rework "terms.rst"
source/elements/oneMKL/source/domains/data_fitting/interpolate.rst
Outdated
Show resolved
Hide resolved
| typename Interpolant::fp_type* sites, | ||
| std::int64_t n_sites, | ||
| typename Interpolant::fp_type* results, | ||
| std::bitset<32> der_indicator, |
There was a problem hiding this comment.
is there any reason you shortened this from derivative_indicator to der_indicator ? The former would make more sense to me since almost everything else is already full named.
There was a problem hiding this comment.
although I suppose you have used "ders" in the interpolate_hint types... so maybe there is some precedence.
There was a problem hiding this comment.
You're right that we use "ders" as a part of interpolation hint names. Am I right that you're ok if we use ders_indicator as a name?
There was a problem hiding this comment.
I suppose so, but if I were you, I would choose a general pattern so it is predictable for all names... lower snake case with full words ... I see you sometimes shortening words sometimes not shortening them... is there a pattern to it? maybe you wan to rethink all the names into a single pattern :)
See if you can write down a general naming pattern: recall this spec is supposed to be written so that another developer could implement the library, so you want to describe naming patterns when applicable in case someone wants to suggest an extension.
There was a problem hiding this comment.
on the other hand, I am ok with just a few shortened cases as long as it is readable... but you should review all your names and see if there is a general pattern that you can (and want to) write down... you may discover you want to be more consistent, or that you are ok with it :) To me this is just one aspect of the full review.
There was a problem hiding this comment.
Ok. I see. Yeah, we need to think about general pattern and exceptions for it (if any). As for me interpolation sites will look too long... However, we definitely need to think about it. Thanks
source/elements/oneMKL/source/domains/data_fitting/interpolate.rst
Outdated
Show resolved
Hide resolved
|
|
||
| template <typename Interpolant> | ||
| sycl::event interpolate( | ||
| Interpolant& interpolant, |
There was a problem hiding this comment.
are these missing the queue as argument?
Also, do you fit into non-member or member function cases as seen in https://github.com/oneapi-src/oneAPI-spec/blob/main/source/elements/oneMKL/source/architecture/execution_model.inc.rst#member-functions ? If you are of member-function approach, you may want to add some language there to include Data fitting in list with DFt and RNG.
There was a problem hiding this comment.
queue is not missed it's inside interpolant (already associated with interpolant). We also have interfaces with "queue" below.
It's a non-member function. Spec says:
Each oneMKL non-member computational routine takes a sycl::queue reference as its first parameter
I see the point. We need to say somehow that the first argument contains execution context.
Given our case maybe we can rephrase the sentence from execution_model.inc.rst:
Each oneMKL non-member computational routine takes a value that contain execution context as its first parameter
What do you think?
There was a problem hiding this comment.
What about changing the spec to say something like this:
In general, each oneMKL non-member computational routine takes a sycl::queue reference as its first parameter.
In some exceptional cases, like in Data Fitting, it may instead take an object as first parameter that itself contains a reference to the sycl execution context.
There was a problem hiding this comment.
why is a queue not passed in? why is a context sufficient ? Is there a future call which adds the queue ? How does work get done ?
There was a problem hiding this comment.
Actually the queue is inside (See here: https://github.com/oneapi-src/oneAPI-spec/pull/413/files/#diff-2361f70703f03fec4684fd287b54dae6e0b303c2f095bab62be9b5bc536b51f8R34-R41).
Sorry, I might misspell it trying to explain it more generally.
| spline( | ||
| const sycl::device& device, | ||
| const sycl::context& context, | ||
| std::int64_t ny = 1, | ||
| bool were_coeffs_computed = false); |
There was a problem hiding this comment.
under what cases is this sufficient? is this constructor when you need to pass the queue in the interpolate function ? What problem is this trying to solve? Running on different queues at the same time with the same internal spline data ? Do you have anywhere in this document that explains the rationale behind having these two constructors for the spline?
There was a problem hiding this comment.
is this constructor when you need to pass the queue in the interpolate function ?
Correct.
Running on different queues at the same time with the same internal spline data ?
It's helpful when users do interpolation in different queues, splitting sites between them: the spline object is the same, but sites are different.
Do you have anywhere in this document that explains the rationale behind having these two constructors for the spline?
Yes. It's here. Maybe we need to describe cases clearly here.
There was a problem hiding this comment.
Yes. It's here. Maybe we need to describe cases clearly here.
Yes, I highly recommend to figure out how to describe the usage models of the spline object and how it can interact with the interpolate functions in a more high level design approach instead of buried in the comments about the constructors :) You can then refer to that usage model discussion/design here. Remember that the target audience for this document is other library designers. You want to explain the what and why (when it is not clear) as much as the how :)
There was a problem hiding this comment.
Agree... Thanks for pointing it out!
|
Hi @capatober, can I recommend you add a graphical picture of interpolation like you have in the oneMKL TAB presentation? That would enhance your api description here. It would probably best belong in the intro to data fitting page, but possibly in the define the terminology page. |
|
Hi @capatober it looks like the ci build is failing in latexpdf step, you may want to try building locally to figure out what is going wrong and fix it |
|
Hi @andreyfe1 can you update on the status of this PR? Do you have a sense of when you want to target getting this in? We are just passing a provisional 1.2 release now and the next one will be around same time as oneMKL 2023.0 product release (in november or so) What needs to be done more for this ? |
|
Hi @spencerpatty! |
|
@andreyfe1 and @paveldyakov any reason why we shouldn't also merge this PR for Spec 1.3 ? |
|
Hi @spencerpatty. As it's mentioned one comment above we are still waiting for feedbacks. That's why we shouldn't also merge this PR for Spec 1.3 |
The PR introduces the new (experimental) domain of oneMKL: Data Fitting. It provides spline-based interpolation capabilities that can be used for spline construction (Linear, Cubic, Quadratic etc.), to perform cell-search operations, and to approximate functions, function derivatives, or integrals.
The PR is addressing the #379 issue