-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Added an extension method for saving statically typed model (#1286) #2924
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
Codecov Report
@@ Coverage Diff @@
## master #2924 +/- ##
=========================================
Coverage ? 72.53%
=========================================
Files ? 809
Lines ? 144746
Branches ? 16204
=========================================
Hits ? 104987
Misses ? 35347
Partials ? 4412
|
Hi @llRandom thanks for the PR! In #2735 we expressed a change in this API that they should accept not only the model, but also some description of the inputs to that model (so as to make certain information discoverable). In the base so-called "dynamic" API, this descriptor takes the form of either:
Note also the new presence of overloads for taking file paths and not just streams, for convenience sake. Note the APIs were removed altogether and put into their final form in #3044, so I might think this code may not build at this time. |
|
||
namespace Microsoft.ML.StaticPipe | ||
{ | ||
public static class ModelOperationsCatalog |
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.
ModelOperationsCatalog [](start = 24, length = 22)
It may be preferable if the static class containing the extension methods for this did not have the same name as the type they're operating on, that is ModelOperationsCatalogExtensions
or suchlike may be preferable.
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.
fixed
/// <param name="catalog">The model explainability operations catalog.</param> | ||
/// <param name="model">The trained model to be saved.</param> | ||
/// <param name="stream">A writeable, seekable stream to save to.</param> | ||
public static void Save<TInShape, TOutShape, TTransformer>(this ML.ModelOperationsCatalog catalog, Transformer<TInShape, TOutShape, TTransformer> model, Stream stream) |
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.
ModelOperationsCatalog catalog [](start = 75, length = 30)
This is an interesting question I'm not sure I know the answer to. The experimental "static" API predates the concept of the MLContext
, and solved the problem MLContext
seeks to solve -- that of discoverability, and (at a lower level) propagation of the IHostEnvironment
, in a rather entirely different way -- that is, you apply estimators and transformers in static-pipe land through discoverability on the input data to those estimators and transformers -- that is, intellisense on the columns.
Dynamic pipe land can't do that -- there is no (and kind of cannot be) concept of a column as "separate object" in dynamic-pipe land. So we achieve discoverability of relevant instantiable components via the taxonomy as originally proposed in #1098.
Here we're, AFAIK for the first time, mixing the two metaphors. This may be appropriate. Indeed I think we might shift the static API to use more of these idioms (which, again, predates MLContext
altogether during the period we were engineering its core principles).
Not asking you to answer that question, just registering my thoughts since this change brought it to my mind, if we continue to iterate on the static pipe...
test/Microsoft.ML.Tests/Scenarios/Api/CookbookSamples/CookbookSamples.cs
Outdated
Show resolved
Hide resolved
# Conflicts: # test/Microsoft.ML.Tests/Scenarios/Api/CookbookSamples/CookbookSamples.cs
…api changes & documentation example
Are there any other changes needed for this PR? |
@TomFinley - were all of your comments addressed? |
The static API has been removed. Closing as this PR is no longer necessary. |
Added an extension method for saving statically typed model
Fixes #1286