Fix loading of old ConcatTransform models #1301
Conversation
… for sizeof(Float)
| @@ -306,6 +306,10 @@ private ColumnInfo[] LoadLegacy(ModelLoadContext ctx) | |||
| // int: index of the alias | |||
| // int: string id of the alias | |||
| // int: -1, marks the end of the list | |||
| // int: sizeof(Float). | |||
There was a problem hiding this comment.
int: sizeof(Float). [](start = 15, length = 19)
why is it at the end? #Resolved
| // int: sizeof(Float). | ||
|
|
||
| var sizeofFloat = ctx.Reader.ReadInt32(); | ||
| Contracts.CheckDecode(sizeofFloat == 4 || sizeofFloat == 8); |
There was a problem hiding this comment.
4 || sizeofFloat == 8 [](start = 49, length = 21)
sizeof(float) and sizeof(double) ? #Resolved
There was a problem hiding this comment.
Actually, how about we disallow the double build? It's not like anyone was using it or something
In reply to: 226423915 [](ancestors = 226423915)
I wonder if we should add a test for the legacy load. Maybe not one transform at a time, but a bigger model with a bunch of transforms? Refers to: src/Microsoft.ML.Data/Transforms/ConcatTransform.cs:295 in 2bd0a6f. [](commit_id = 2bd0a6f, deletion_comment = False) |
No matter what - we should add at least 1 test here in this PR. You can follow the same approach I took for #1290 and add the model to https://github.com/dotnet/machinelearning-testdata/tree/master/Microsoft.ML.TestModels/TestModels In reply to: 431120918 [](ancestors = 431120918) Refers to: src/Microsoft.ML.Data/Transforms/ConcatTransform.cs:295 in 2bd0a6f. [](commit_id = 2bd0a6f, deletion_comment = False) |
|
@yaeldekel Should we merge it? |
ghost
locked as resolved and limited conversation to collaborators
Fixes #1300 .