Additional changes to ExpressionTransformer #4614
Conversation
There was a problem hiding this comment.
LGTM, I linked the code fixes to the original comments; mainly to help me understand what comments were being addressed.
The addressed parts look good.
I see these comments remaining in the original PR:
- Vector column handling -- Expression estimator/transformer #4548 (comment)
- ONNX -- Expression estimator/transformer #4548 (review)
| | long | convert to <xref:System.Int64> | The input may be of any type. | | ||
| | single, float | convert to <xref:System.Single> | The input may be of any type. | | ||
| | double | convert to <xref:System.Double> | The input may be of any type. | | ||
| | text | convert to [text](xref:Microsoft.ML.Data.TextDataViewType) | The input may be of any type. This produces a default text representation. | |
There was a problem hiding this comment.
Addresses: #4548 (comment)
#Resolved
|
|
||
| // Since the abs of the base is at least two, the exponent must be less than 31. | ||
| if (b >= 31) | ||
| throw Contracts.Except("Cannot raise an integer to a power greater than 30"); | ||
| return neg ? I4.MinValue : I4.MaxValue; |
There was a problem hiding this comment.
Addresses: #4548 (comment)
#Resolved
| Param, | ||
| Auto, | ||
|
|
||
| // REVIEW: These are specific to the ExprTransform parser. Use a general mechanism. |
There was a problem hiding this comment.
Addresses: #4548 (comment)
#Resolved
| /// [!code-csharp[Expression](~/../docs/samples/docs/samples/Microsoft.ML.Samples/Dynamic/Transforms/Expression.cs)] | ||
| /// ]]> | ||
| /// </format> | ||
| /// </example> |
There was a problem hiding this comment.
Addresses: #4548 (comment)
#Resolved
|
Thanks for the quick review! In reply to: 337484693 [](ancestors = 337484693) |
Codecov Report
@@ Coverage Diff @@
## master #4614 +/- ##
==========================================
+ Coverage 74.88% 75.64% +0.76%
==========================================
Files 935 938 +3
Lines 168440 168623 +183
Branches 18189 18213 +24
==========================================
+ Hits 126132 127562 +1430
+ Misses 37325 36025 -1300
- Partials 4983 5036 +53
|
| Norm, | ||
| FloatsFromBytes, | ||
| Param, | ||
| Auto, |
There was a problem hiding this comment.
See: #4548 (comment)
These are used for the internal NN language, Net#. If we are no longer supporting that internally, then they are no longer needed. I don't think this PR provides the code for those functions, like convolution.
We may want to check that abs(a), min(a, b), max(a,b ), sqrt(a) still work.
ghost
locked as resolved and limited conversation to collaborators
This PR addresses the ExpressionTransformer part of issue #4610 and also addresses some left over comments from PR #4548 .