-
Notifications
You must be signed in to change notification settings - Fork 1.9k
CountFeatureSelectingEstimator no selection support #5000
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
@@ -896,27 +896,25 @@ public void SaveAsOnnx(OnnxContext ctx) | |||
public bool SaveAsOnnxCore(OnnxContext ctx, int iinfo, string srcVariableName, string dstVariableName) | |||
{ | |||
string opType; | |||
if (_srcTypes[iinfo] is VectorDataViewType) | |||
var slots = _slotDropper[iinfo].GetPreservedSlots(); | |||
if (_srcTypes[iinfo] is VectorDataViewType && slots.Count() > 0) |
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.
_srcTypes[iinfo] is VectorDataViewType && slots.Count() > 0 [](start = 20, length = 59)
what if _srcTypes[iinfo] is VectorDataViewType && slots.Count() == 0? should we return true or false?
in current implementation seems it will goes into the "else" condition in line 917 and which should handling double numeric data type #Resolved
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.
If slots.Count == 0, then the column get's "suppressed". It's not handled well by the GatherElements operator, so we create an empty vector in the else clause. Does that make sense?
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.
yes, but we better to make the logic clear like below, don't let different cases fall into same condition which will make code harder to read and will cause potential issue in future:
if (_srcTypes[iinfo] is VectorDataViewType)
{
if (count > 0) { ... }
else { ... }
}
else { ... }
In reply to: 404404988 [](ancestors = 404404988)
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.
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.
Issue: The GatherElements ONNX operator used by SlotsDroppingTransformer (transformer for CountFeatureSelectingEstimator) has issues when there's nothing selected, so mlnet needs to handle this case separately.
This PR
Other issues: