Adding a new CI leg for netcoreapp 3.0 #1700
Conversation
Anipik
changed the title
|
@danmosemsft @safern @Ivanidzo4ka @shauheen @tannergooding can please take a look at this pr ? |
| @@ -158,9 +158,6 @@ public static unsafe void MatMul(AlignedArray mat, AlignedArray src, AlignedArra | |||
|
|
|||
| public static unsafe void MatMul(ReadOnlySpan<float> mat, ReadOnlySpan<float> src, Span<float> dst, int crow, int ccol) | |||
| { | |||
| Contracts.Assert(crow % 4 == 0); | |||
There was a problem hiding this comment.
Why are these asserts being removed as part of enabling CI?
There was a problem hiding this comment.
These asserts are wrong, they were not being tested earlier because contracts.Assert was being removed.
| @@ -161,7 +161,7 @@ public void EarlyStoppingTest() | |||
| /// <summary> | |||
| /// Multiclass Logistic Regression test. | |||
| /// </summary> | |||
| [Fact] | |||
| [ConditionalFact(typeof(BaseTestBaseline), nameof(BaseTestBaseline.NotNetCore30))] // netcore3.0 output differs from Baseline | |||
There was a problem hiding this comment.
Disabling these tests on netcoreapp3.0 feels wrong. What happens when we completely move to .NET Core 3.0? We will then have to spend effort on re-enabling these tests for 3.0.
Is there a way we can investigate why the baselines differ? Rather than just shutting the tests off?
There was a problem hiding this comment.
I am working on investigating the failures on netcoreapp3.0
I was able to fix some more tests in the aligned Array PR,
I have an issue open with the list of those tests. but we may require some more people to work on it.
|
@eerhardt can you take another look ? |
| @@ -254,7 +254,7 @@ public void TestCrossValidationBinaryMacro() | |||
| } | |||
| } | |||
|
|
|||
| [Fact] | |||
| [ConditionalFact(typeof(BaseTestBaseline), nameof(BaseTestBaseline.LessThanNetCore30OrNotNetCore))] // netcore3.0 output differs from Baseline | |||
There was a problem hiding this comment.
What are we doing to track getting these tests enabled on .net core 3?
There was a problem hiding this comment.
i have opened the issue for it #1096
It contains the list of tests that we fixed and ones still remaining
|
Release build is still failing |
it is due to this change #1724 |
fixing new tests broken by other commits Fixing Tests
| @@ -951,8 +943,6 @@ public static unsafe void Scale(float scale, Span<float> dst) | |||
|
|
|||
| public static unsafe void ScaleSrcU(float scale, ReadOnlySpan<float> src, Span<float> dst, int count) | |||
| { | |||
| Contracts.Assert(src.Length == dst.Length); | |||
There was a problem hiding this comment.
I think asserts here are still valuable, aren't they?
Contracts.Assert(count <= src.Length && src.Length <= dst.Length);
There was a problem hiding this comment.
I will add the more appropiate asserts with the documentation pr.
I am thinking of an another approach of slicing the span to the required length before passing to these functions and adding the contracts.Assert(src.Length == dst.Length) . Is this approach worth doing ?
There was a problem hiding this comment.
IMO - no I don't think that approach is worth doing. That's why the count parameter is passed to these functions - so the spans don't need to be the same length, and one of the spans isn't chosen as "the one with the right length". It's unnecessary logic to ensure the spans are the same length.
Slicing spans is cheap, but it isn't free. You still have bounds checks to ensure the sliced values are less than the span length.
ghost
locked as resolved and limited conversation to collaborators
Fixes #1711