Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[oneMKL] add Data Fitting domain to oneMKL #413
[oneMKL] add Data Fitting domain to oneMKL #413
Changes from 1 commit
85c5505
3bea0b8
f0cad28
204a15e
9e6e933
39147fc
ddceee5
10f36ac
79582bf
9139dc3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems different than your definition of site in the terms.rst
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct.
Statements here are correct. Will rework "terms.rst"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
queue is not missed it's inside
interpolant
(already associated withinterpolant
). We also have interfaces with "queue" below.It's a non-member function. Spec says:
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
:What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about changing the spec to say something like this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any reason you shortened this from
derivative_indicator
toder_indicator
? The former would make more sense to me since almost everything else is already full named.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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