Repository navigation
Fix/green ci - #16
Merged
Merged
Fix/green ci#16
Conversation
Numerics.sln still listed CSharpNumerics.Engines and NumericTest.Engines, which moved to their own repository in e98465b. dotnet build Numerics.sln therefore fails on a clean checkout with MSB3202. That went unnoticed because nothing builds the solution. publish.yml builds and packs Numerics/Numerics/CSharpNumerics.csproj directly, and it runs only on version tags. The same gap has a second consequence worth naming: publish.yml has no dotnet test step at all, so a release can be published to NuGet without a single test having been executed. Locally the suite is blocked by a Windows application control policy that prevents locally built test assemblies from loading, so in practice the tests have not been running anywhere. Removes the two dead project entries and adds a CI workflow that builds the solution and runs the suite on every push and pull request. It does not publish, and publish.yml is left alone - adding a test gate there is a change to the release path and should be a deliberate decision.
Two failures that the new CI surfaced. Both predate it; nothing had run the suite before. TestMandelbrot constructs a System.Drawing Bitmap, and System.Drawing.Common is Windows-only from .NET 6 onward, so it can never pass on a Linux runner. It now reports inconclusive off Windows rather than failing. Inconclusive rather than skipped because the test is not irrelevant there - it simply cannot run - and the Mandelbrot mathematics is already covered by the complex number tests beside it. The alternative, moving the whole suite onto a Windows runner for one rendering test, would be a poor trade for a library that otherwise targets netstandard2.1. StratifiedKFoldCrossValidator is a real bug in shipped code, and this commit does not fix it. Folds are filled class by class with each class restarting at fold 0, so a class with fewer members than the fold count leaves later folds short, and small classes throughout leave them empty. An empty fold then reaches the VectorN constructor and fails with 'values cannot be null or empty', which says nothing about the cause. I could not reproduce the root cause by reading: the test generates 100 samples in two balanced classes across five folds, which should give twenty per fold and no empty one. Since the suite cannot be executed locally - a Windows application control policy blocks locally built test assemblies - guessing at a fix for cross-validation code that users are already running would be worse than leaving it visible. So this adds a guard that fails with the sample count, the class count and the smallest class size. The test still fails, deliberately, but the next CI run reports what the fold distribution actually is, which is what is needed to fix it properly.
Root cause of the stratified k-fold failure CI surfaced, and it is worse than the failing test. Series.FromCsv returned Cols as header.Skip(1). The idiom was copied from TimeSeries.FromCsv, where column 0 is the time axis and genuinely is excluded from Data. Series has no time axis, so every name was shifted one step left of the column it described, and IndexOf(Cols, name) pointed at the wrong data column. The stratified test was the loud symptom: its y became a continuous feature instead of the class labels, GroupBy produced a hundred singleton classes, the per-class round-robin put every sample in fold 0, and the empty training set for that fold crashed the VectorN constructor. The quiet symptom matters more. RollingCrossValidator and LeaveOneOutCrossValidator resolve the target through the same lookup, so every cross-validation run on a Series loaded from CSV has been training with the target leaked into the features and validating against a feature column - producing plausible-looking scores. The regression test in the existing suite asserted BestScore > -10, which nonsense results clear comfortably. Cols is now the names of exactly the columns present in Data, in order, whatever was excluded. TimeSeries.FromCsv is untouched: its Skip(1) is correct. The stratified test now excludes the target from X via the same looked-up index instead of a hardcoded one, and a regression test pins the Cols-to-Data alignment directly.
…o end RunBootstrap_SameSeed_ShouldBeReproducible failed in CI with two seeded runs differing by 0.014. The test has always been flaky; it was exposed, not broken, by the recent changes. MonteCarloClustering.Seed seeded the bootstrap sampling and nothing else. The model clone fitted inside each iteration kept its own seed, and the test's KMeans had none - so every centroid initialisation ran off new Random(), which draws a fresh nondeterministic seed per instance. Two identical seeded runs therefore agreed only as long as k-means happened to land in the same local optima both times. On well-separated data that is usually true, which is why the test mostly passed, and 'mostly' is the definition of a flaky test. A seeded run now hands each model clone a seed derived from the run's own generator, through the SetHyperParameters mechanism the models already have. Derived per iteration rather than fixed, so bootstrap replicates keep independent initialisations. Models that take no seed, like DBSCAN, ignore the key. Unseeded runs are untouched. Exact numbers from previously seeded runs will differ, since the generator now also feeds the model seeds. Nothing can have depended on the old values - the model initialisation was nondeterministic, which is the bug.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.