-
Notifications
You must be signed in to change notification settings - Fork 75
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
Matrix operators #398
Matrix operators #398
Conversation
Codecov Report
@@ Coverage Diff @@
## master #398 +/- ##
==========================================
- Coverage 80.61% 80.57% -0.04%
==========================================
Files 145 145
Lines 12451 12512 +61
Branches 1676 1704 +28
==========================================
+ Hits 10037 10082 +45
- Misses 1902 1919 +17
+ Partials 512 511 -1
Continue to review full report at Codecov.
|
Moving PeterONumbers from 1.7.4 to 1.8 broke a couple of tests, 🦆 |
[Fact] public void PowI_2() => Test(Pow(i, 2), "-1"); | ||
[Fact] public void PowI_I() => Test(Pow(i, i), "0.2078795763507619085469556198349787700338778416317696080751358830554198772854821397886002778654260353"); | ||
[Fact] public void Pow0D5M0D8I_0D5() => Test(Pow(0.5m-0.8m*i, 0.5m), "0.8495287261787150364907695626370349126367049237188233590072351305588742785045041120988684216395800850 - 0.4708492928770629369416080841473292873459093837847060526082076263776372676635818831502937646218684434i"); | ||
[Fact] public void Sqrt0D5M0D8I() => Test(Sqrt(0.5m-0.8m*i), "0.8495287261787150364907695626370349126367049237188233590072351305588742785045041120988684216395800853 - 0.4708492928770629369416080841473292873459093837847060526082076263776372676635818831502937646218684426i"); | ||
[Fact] public void Pow3P2I_3M2I() => Test(Pow(3+2*i,3-2*i), "105.7489754858859776057429151808913270583911891854438642007164406579203854865458413612560669739008053 - 109.0885464095191985068230453514141449073149001480712525014939233398460315014292268679635166428957469i"); |
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 all stuff... due to some precision regression in PeterO.Numbers 1.8.0 (as compared to 1.7.4, the previous version). Should be reported to their repo.
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.
Then is there a reason to update?
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.
Just because updated is better than non-updated?
public void TestInverseWithGT101Issue400(string matrixRaw) | ||
{ | ||
Matrix a = matrixRaw; | ||
var inverse = a.Inverse!; |
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.
Make a ThrowWhenNull
as specified under https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/attributes/nullable-analysis#specify-post-conditions-maybenull-and-notnull
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.
ThrowWhenNull
is an attribute for return
though. null
is a perfectly valid return from Inverse
. So I'll just make a Assert.NotNull
(I actually thought that !
throws an exception!)
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.
ThrowWhenNull
is not an attribute, and [NotNull]
can be specified at the argument position as documented.
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 can check, now it's AsNotNull<>
.
AngouriMath/Sources/Tests/UnitTests/TestUtils.cs
Lines 18 to 22 in 4043999
public static T AsNotNull<T>(T? value) | |
{ | |
Assert.NotNull(value); | |
return value!; | |
} |
[Fact] public void PowI_2() => Test(Pow(i, 2), "-1"); | ||
[Fact] public void PowI_I() => Test(Pow(i, i), "0.2078795763507619085469556198349787700338778416317696080751358830554198772854821397886002778654260353"); | ||
[Fact] public void Pow0D5M0D8I_0D5() => Test(Pow(0.5m-0.8m*i, 0.5m), "0.8495287261787150364907695626370349126367049237188233590072351305588742785045041120988684216395800850 - 0.4708492928770629369416080841473292873459093837847060526082076263776372676635818831502937646218684434i"); | ||
[Fact] public void Sqrt0D5M0D8I() => Test(Sqrt(0.5m-0.8m*i), "0.8495287261787150364907695626370349126367049237188233590072351305588742785045041120988684216395800853 - 0.4708492928770629369416080841473292873459093837847060526082076263776372676635818831502937646218684426i"); | ||
[Fact] public void Pow3P2I_3M2I() => Test(Pow(3+2*i,3-2*i), "105.7489754858859776057429151808913270583911891854438642007164406579203854865458413612560669739008053 - 109.0885464095191985068230453514141449073149001480712525014939233398460315014292268679635166428957469i"); |
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.
Then is there a reason to update?
.Substitute("B", 2) | ||
.Substitute("C", 3) | ||
.Substitute("D", 4); | ||
Assert.Equal(-2, v.EvalNumerical()); | ||
Assert.NotNull(v); | ||
Assert.Equal(-2, v!.EvalNumerical()); |
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.
Make a ThrowWhenNull
as specified under https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/attributes/nullable-analysis#specify-post-conditions-maybenull-and-notnull
public Entity? Determinant => determinant.GetValue( | ||
static @this => | ||
{ | ||
if ([email protected]) |
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.
Shouldn't this check be in GenericTensor?
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.
Yes, but it is exception-based. Here we want to return a null in that case. (sure we can catch exceptions, but... quack. Maybe we will after the GenericTensorBaseException
implemented)
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.
Then don't throw exceptions in GenericTensor!
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.
if ([email protected]) | ||
return null; | ||
if (@this.Determinant is null) | ||
return null; | ||
if (@this.Determinant == 0) |
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.
Shouldn't these checks be in GenericTensor?
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.
Same thing, we want to return null to avoid an exception
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.
Then don't throw exceptions in GenericTensor!
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.
Hmmmmmmmmmmmmmmmmmmmmmmmmmmm. We can have 1.0.1-noexcept
package for it. Since if you remember, there's ALLOW_EXCEPTION
flag.
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.
@MomoDeve would love this
(* | ||
|
||
Matrix and matrix operators | ||
|
||
*) |
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 can be just a single-line comment
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 was trying to make it "bold" :).
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.
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.
Lol
(* | ||
|
||
Matrix and scalar operators | ||
Scalar and matrix operators | ||
|
||
*) |
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.
Can be two single-line comments
|+
,|-
,|*
,|/
,|**
will return an instance ofMatrix
, no matter what. Any feedback on the API?