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.

Sign up for GitHub

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

Jump to bottom

[DFT] Constrain data_type in compute_*ward real-real inplace #544

Merged
merged 3 commits into from
Jul 2, 2024
Merged

[DFT] Constrain data_type in compute_*ward real-real inplace #544

merged 3 commits into from
Jul 2, 2024

Conversation

hjabird
Copy link
Contributor

@hjabird hjabird commented Jun 10, 2024

  • The compute_*ward real-real implace overload can be selected when the user intends to use the complex-complex out-of-place overload.
  • This suggestion stops this from happening when the user uses non-float arguments that are presumably intended for the complex-complex overload.

For a complete description of the issue, see oneapi-src/oneMKL#499, and also PR with this change to the spec adopted at oneapi-src/oneMKL#503.

@hjabird
[DFT] Constrain data_type in compute_*ward real-real inplace
8fc18df 
* The compute_*ward real-real implace overload can be selected when the user
intends to use the complex-complex out-of-place overload.
* This suggestion stops this from happening when the user uses non-float
arguments that are presumably intended for the complex-complex overload.
@hjabird
Copy link
Contributor Author

hjabird commented Jun 17, 2024

@Rbiessy @lhuot Can you check that you're happy with my changes and resolve the comments when you have time? Thanks!

@hjabird
Copy link
Contributor Author

hjabird commented Jun 24, 2024

@lhuot If you're happy with this, can you approve it?

lhuot
lhuot approved these changes Jun 24, 2024
View reviewed changes
Copy link
Contributor

@lhuot lhuot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@hjabird
Copy link
Contributor Author

hjabird commented Jun 25, 2024

@lhuot @rscohn2 @Rbiessy I think this spec change is minor enough that we can merge it without presenting it to the Math SIG. Outside of this PR, it has also been discussed in oneapi-src/oneMKL#503.

@lhuot
Copy link
Contributor

lhuot commented Jun 25, 2024

@lhuot @rscohn2 @Rbiessy I think this spec change is minor enough that we can merge it without presenting it to the Math SIG. Outside of this PR, it has also been discussed in oneapi-src/oneMKL#503.

Yes that sounds reasonable to me too. (fyi I can't merge)

@hjabird
Copy link
Contributor Author

hjabird commented Jun 27, 2024

@rscohn2 are you happy to merge this?

@hjabird
Copy link
Contributor Author

hjabird commented Jul 2, 2024

We're still looking for someone to merge this. @rscohn2, @mmeterel, @andrewtbarker it looks like you have merge powers?

@Rbiessy
Copy link
Contributor

Rbiessy commented Jul 2, 2024

Maybe @spencerpatty too?

@rscohn2 rscohn2 merged commit 2c09ad3 into uxlfoundation:main Jul 2, 2024
3 checks passed
@rscohn2
Copy link
Member

rscohn2 commented Jul 2, 2024

We can give more people merge rights. It is up to the owners of the mkl spec.

@hjabird hjabird mentioned this pull request Jul 2, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lhuot lhuot lhuot approved these changes

@Rbiessy Rbiessy Rbiessy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hjabird @lhuot @Rbiessy @rscohn2