Skip to content

Unconstraining Scalar ; adding BigInteger and Complex #633

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
Merged
merged 13 commits into from
Oct 17, 2021
Merged

Unconstraining Scalar ; adding BigInteger and Complex #633

merged 13 commits into from
Oct 17, 2021

Conversation

WhiteBlackGoose
Copy link
Contributor

Summary of the PR

Unconstraining T in Scalar<> and adding BigInteger and Complex

@WhiteBlackGoose WhiteBlackGoose changed the title [WIP] Improving Scalar [WIP] Unconstraining Scalar ; adding BigInteger and Complex Oct 11, 2021
@WhiteBlackGoose WhiteBlackGoose marked this pull request as ready for review October 11, 2021 13:12
@WhiteBlackGoose WhiteBlackGoose changed the title [WIP] Unconstraining Scalar ; adding BigInteger and Complex Unconstraining Scalar ; adding BigInteger and Complex Oct 11, 2021
@WhiteBlackGoose
Copy link
Contributor Author

I haven't worked on Scalar.MathFPort and MissingMethods for now

HurricanKai
HurricanKai requested changes Oct 11, 2021
View reviewed changes
Copy link
Member

@HurricanKai HurricanKai left a comment

Choose a reason for hiding this comment

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

Initial review. I need to inform myself what ie Reciprocal of a Complex even means.

src/Maths/Silk.NET.Maths/Scalar.Constants.cs
else if (typeof(T) == typeof(decimal))
{
Epsilon = default;
Epsilon = default!;
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't feel right, are you sure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean, that's how it was. I just added a ! (which suppresses a warning, doesn't do anything beyond that)
image

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I know. But where does the nullability analysis warning come from? Is it not new?

Copy link
Contributor Author

@WhiteBlackGoose WhiteBlackGoose Oct 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

From T unconstrained. It was unmanaged, so nobody cared about it. Now it's potentially a ref type

HurricanKai
HurricanKai reviewed Oct 11, 2021
View reviewed changes
@WhiteBlackGoose WhiteBlackGoose marked this pull request as draft October 11, 2021 13:44
@WhiteBlackGoose
Copy link
Contributor Author

WhiteBlackGoose commented Oct 11, 2021 •
edited
Loading

Oopsi, I forgot to change the docs

@WhiteBlackGoose WhiteBlackGoose marked this pull request as ready for review October 11, 2021 13:46
@WhiteBlackGoose
Copy link
Contributor Author

Okay so there was no docs to change 😆 🦆🦆🦆

@Perksey Perksey added area-Maths enhancement New feature or request labels Oct 11, 2021
HurricanKai
HurricanKai reviewed Oct 16, 2021
View reviewed changes
Copy link
Member

@HurricanKai HurricanKai left a comment

Choose a reason for hiding this comment

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

Other then that one discussion this seems good, am I mistaken or does this only include Reciprocal and Abs? Happy to have those of course, I would just not return true in IsSupported for those types.

@WhiteBlackGoose
Copy link
Contributor Author

WhiteBlackGoose commented Oct 16, 2021 •
edited
Loading

Uh, no, there's also BaseOps changed :) @HurricanKai

@WhiteBlackGoose
Copy link
Contributor Author

WhiteBlackGoose commented Oct 16, 2021 •
edited
Loading

It's just not expanded by GH's Files Changed tab

@HurricanKai
Copy link
Member

Oh, why is that, where can I view those changes then?

@WhiteBlackGoose
Copy link
Contributor Author

@HurricanKai easy to miss :)
D8E94D11-5E90-43B6-A8D9-7182004BF0D5

@HurricanKai
Copy link
Member

Fantastic, thank you.
UI in the app is even better: Screenshot_20211016-132214-080.png

HurricanKai
HurricanKai reviewed Oct 16, 2021
View reviewed changes
src/Maths/Silk.NET.Maths/Scalar.BaseOps.cs Outdated
/// <returns><code>true</code> if the value is normal; <code>false</code> otherwise.</returns>
[MethodImpl(MaxOpt)]
public static bool IsNormal<T>(T f) where T : unmanaged
public static bool IsNormal<T>(T f)
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised a Complex can't be normal, that seems odd. I know S.Numerics doesn't have it either. Do you have an idea why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh, I don't know 😅 . I can add it as Normal of its imaginary & real parts, but I don't know if it makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

Eh, let's just not then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If something is not added, it can be added later 🤷

HurricanKai
HurricanKai reviewed Oct 16, 2021
View reviewed changes
Copy link
Member

@HurricanKai HurricanKai left a comment

Choose a reason for hiding this comment

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

Can you also change the constraints for Sin and such?

src/Maths/Silk.NET.Maths/Scalar.BaseOps.cs Outdated
#endif
#if SSE
using System.Runtime.Intrinsics.X86;
// ReSharper disable CompareOfFloatsByEqualityOperator
Copy link
Member

Choose a reason for hiding this comment

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

Moved out of the #if please

HurricanKai
HurricanKai approved these changes Oct 17, 2021
View reviewed changes
Copy link
Member

@HurricanKai HurricanKai left a comment

Choose a reason for hiding this comment

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

Looking good. Just missing Scalsr and a bunch of MathFPort stuffs for full support 🙌

@HurricanKai HurricanKai merged commit d0e4914 into dotnet:main Oct 17, 2021
Perksey pushed a commit that referenced this pull request Nov 20, 2023
@WhiteBlackGoose
Unconstraining Scalar ; adding BigInteger and Complex (#633)
65a2280 
* improving math

* Unconstrained

* warnings suppressed

* WIP: Adding BigInteger and Complex

* Keeping adding

* #if-ed unavailable API

* Other unavailable API #ifed

* I love typos

* Abs for Complex added. Tests

* Tests added

* notnull constraint added

* Suppress moved out of #if

* MathFPort constraints changed to notnull
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HurricanKai HurricanKai HurricanKai approved these changes

Assignees

No one assigned

Labels

area-Maths enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@WhiteBlackGoose @HurricanKai @Perksey