-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Check for IEstimator/ITransformer schema consistency, fix bugs uncovered #3408
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
Changes from all commits
888fb85
07aded5
f4686fb
2a20421
fc05d07
36953ab
bc4318e
922014d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -151,14 +151,18 @@ protected void TestEstimatorCore(IEstimator<ITransformer> estimator, | |
private void CheckSameSchemaShape(SchemaShape promised, SchemaShape delivered) | ||
{ | ||
Assert.True(promised.Count == delivered.Count); | ||
var sortedCols1 = promised.OrderBy(x => x.Name); | ||
var sortedCols2 = delivered.OrderBy(x => x.Name); | ||
var promisedCols = promised.OrderBy(x => x.Name); | ||
var deliveredCols = delivered.OrderBy(x => x.Name); | ||
|
||
foreach (var (x, y) in sortedCols1.Zip(sortedCols2, (x, y) => (x, y))) | ||
foreach (var (p, d) in promisedCols.Zip(deliveredCols, (p, d) => (p, d))) | ||
{ | ||
Assert.Equal(x.Name, y.Name); | ||
Assert.Equal(p.Name, d.Name); | ||
// We want the 'promised' metadata to be a superset of 'delivered'. | ||
Assert.True(y.IsCompatibleWith(x), $"Mismatch on {x.Name}"); | ||
Assert.True(d.IsCompatibleWith(p), $"Mismatch on {p.Name}, there was a mismatch, or some unexpected annotations was present."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm trying to come up with some example where this wouldn't be true.... But nothing comes to my mind, and since tests are not failing, I would guess this is our current state of annotation business. #ByDesign There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Maybe. The way the test worked, it made sure the promise was a superset of what was delivered -- that is, the estimator's annotation list would be a sort of "semi-promise" like, "ok, this annotation may be there or not, but if it is there, it will have this type," and so on. At least, so I must interpret this test as previously written. There is one case in which this could potentially have been valuable, the key-to-vector annotation, as discussed in depth in issue #3380 (the special case where it was a known size vector that was of length precisely one). But that was such an obscure situation and such a micro-optimization that I hardly felt it worthwhile. And anyway, the I could have imagined a "richer" schema shape where there were differing degrees of certainty -- "this will definitely be here, this might be here," etc., but in the end I decided with saying, "you know what, let's just make it a simple promise." One nice thing is that |
||
// We also want the 'delivered' to be a superset of 'promised'. Since the above | ||
// test must have worked if we got this far, I believe the only plausible reason | ||
// this could happen is if there was something promised but not delivered. | ||
Assert.True(p.IsCompatibleWith(d), $"Mismatch on {p.Name}, something was promised in the annotations but not delivered."); | ||
} | ||
} | ||
|
||
|
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.
Curious why this is needed?
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.
Well, it was just something I noticed, that we were propagating that we were going to do this even if it wasn't the right type, which actually led to a test failure. So I just changed the condition to be a bit tighter is all.