-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Term estimators in static pipeline #870
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
Conversation
@@ -327,5 +327,43 @@ public void NormalizerWithOnFit() | |||
Console.WriteLine(Encoding.UTF8.GetString(stream.ToArray())); | |||
} | |||
} | |||
|
|||
[Fact] | |||
public void ToKey() |
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.
ToKey [](start = 20, length = 5)
Do you keep insisting that all the pigsty testing is done in this file?
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.
No, not insisting, it is just useful to me for this to be the case since as far as I see visual studio lost the ability to group tests by anything other than the class, and I am often wanting to run all pigsty using tests (to ensure that when I change the infrastructure I don't break things). I would be more enthusiastic about putting the tests somewhere esle if there was some mechanism to select all tests of a given category, which regrettably I do not see is possible.
In reply to: 216414931 [](ancestors = 216414931)
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 may be something we want to bring up -- we used to have tests categories pretty well lined up across the different things you might want to test -- but now that mechanism seems to be gone, at least from the VS UI...
In reply to: 216490385 [](ancestors = 216490385,216414931)
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.
public delegate void OnFit(ToKeyFitResult<T> result); | ||
|
||
// At the moment this is empty. Once PR #863 clears, we can change this class to hold the output | ||
// key-values metadata. |
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 we have issue about it?
Potentially controversial moves:
TermEstimator
asToKey
, pursuant to There are two transforms with the Friendly Name "Term Transform" #214 discussion. Please consider discussing that there..tt
file to generate the extension methods. We've previously never used T4, but in this case I felt it was warranted.At the moment, #863 is not yet in, so the
onFit
delegate returns an object with nothing in it. If that goes in prior to when we must finish this work I'll update it, otherwise we'll do it later.