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

Add reciprocal and SinCos methods to IFloatingPoint #58607

Closed
Tracked by #63548
lilinus opened this issue Sep 3, 2021 · 8 comments · Fixed by #59521
Closed
Tracked by #63548

Add reciprocal and SinCos methods to IFloatingPoint #58607

lilinus opened this issue Sep 3, 2021 · 8 comments · Fixed by #59521
Labels
api-approved API was approved in API review, it can be implemented area-System.Numerics
Milestone

Comments

@lilinus
Copy link
Contributor

lilinus commented Sep 3, 2021

Background and Motivation

Suggestion for the generic math. The mentioned methods are available in Math and MathF classes. I think it would be nice to add these to the IFloatingPoint interface.

Proposed API

namespace System
{
    public interface IFloatingPoint<TSelf>
    {
...
+        (TSelf, TSelf) SinCos(TSelf x);
+        TSelf ReciprocalEstimate(TSelf x);
+        TSelf ReciprocalSqrtEstimate(TSelf x);
...
    }
}

Oops forgot to add api suggestion label...

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Numerics untriaged New issue has not been triaged by the area owner labels Sep 3, 2021
@ghost
Copy link

ghost commented Sep 3, 2021

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and Motivation

Suggestion for the generic math. The mentioned methods are available in Math and MathF classes. I think it would be nice to add these to the IFloatingPoint interface.

Proposed API

namespace System
{
    public interface IFloatingPoint<TSelf>
    {
...
+        (TSelf, TSelf) SinCos(TSelf x);
+        TSelf ReciprocalEstimate(TSelf x);
+        TSelf ReciprocalSqrtEstimate(TSelf x);
...
    }
}
Author: lilinus
Assignees: -
Labels:

area-System.Numerics, untriaged

Milestone: -

@huoyaoyuan
Copy link
Member

They are more about low-level optimization, when you know the hardware support for some specific type, and looks unsuitable for generic. The interface should contain primitive methods only.

@tannergooding
Copy link
Member

They are more about low-level optimization, when you know the hardware support for some specific type, and looks unsuitable for generic. The interface should contain primitive methods only.

The functions exist as more than just hardware optimizations and they are fully suitable for use in general algorithms, hence why they are on Math/MathF and not some other type.

These were simply missed from the initial preview due to the functions being new in .NET 6.

@tannergooding tannergooding added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner labels Sep 7, 2021
@jeffhandley jeffhandley added this to the 7.0.0 milestone Sep 15, 2021
@bartonjs
Copy link
Member

bartonjs commented Sep 21, 2021

Video

Looks good as proposed.

namespace System
{
    public interface IFloatingPoint<TSelf>
    {
+        (TSelf Sin, TSelf Cos) SinCos(TSelf x);
+        TSelf ReciprocalEstimate(TSelf x);
+        TSelf ReciprocalSqrtEstimate(TSelf x);
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Sep 21, 2021
i3arnon added a commit to i3arnon/runtime that referenced this issue Sep 23, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Sep 23, 2021
@danmoseley
Copy link
Member

@lilinus are you interested in working on this one?

@tannergooding
Copy link
Member

This is one that isn't easy to pick up at the moment since it impacts generic math.

We're working through several parallel changes to get it in box before additional work/changes can happen here.

@lilinus
Copy link
Contributor Author

lilinus commented Mar 15, 2022

Sure, I can pick it up if @i3arnon who has a PR doesn't want to. Will keep an eye out for when generic math is potentially ready for this change.

@i3arnon
Copy link
Contributor

i3arnon commented Mar 15, 2022

@lilinus Thanks for the heads up.
I indeed want it but AFAICT my existing PR is good enough at this moment.
I can update it accordingly when the other generic math changes are done.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 27, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Apr 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Numerics
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants