search

dotnet / machinelearning

ML.NET is an open source and cross-platform machine learning framework for .NET.
https://dot.net/ml
MIT License
9.02k stars 1.88k forks source link

[LoadImages estimator] Set useImageType: false as by default value #4313

Closed CESARDELATORRE closed 4 years ago

CESARDELATORRE commented 5 years ago

In the current preview, in the LoadImages estimator, the parameter useImageType is by default true meaning that it'd use ImageDataView type. false indicates we want the image as a VBuffer.

Since the ImageClassification API needs the image as VBuffer (ImageDataView type is only used for other existing/older scenarios with transforms like resize image, extract pixels etc...) it makes sense to set the parameter useImageType to false as default (to use VBuffer).

However, that change will be convenient for the new ImageClassification API but it would break older code using the LoadImages estimator. Right?

We might need to decide what scenarios we want to favor here for this default value...

harshithapv commented 4 years ago

Originally, the intention of defaulting useImageType to true, was to not break the older code that loaded image as ImageDataView type. However, changing the default will not effect the older code. I have used function overloading wherever necessary. So, there is no preference for what should be the default.

eerhardt commented 4 years ago

What if we used different methods for the two scenarios? Would that help here?

harshithapv commented 4 years ago

Something like LoadImageAsImageDataView and one for LoadImageAsVBuffer? This may break the old code. Or did you mean have current LoadImages Estimator always return ImageDataViewType and have a separate method for VBuffer type.

eerhardt commented 4 years ago

Something like LoadImageAsImageDataView and one for LoadImageAsVBuffer? This may break the old code.

Whenever we have already shipped a public, "stable" API, we shouldn't be breaking it. So the method we already shipped should not change.

Concretely, I meant leave the LoadImages method that shipped in previous stable versions as-is. And create a new method for loading images as VBuffer types.