fix: fix the validation_pack when multiple input #1212
Conversation
| IEnumerable<Tensor> x, | ||
| Tensor y, | ||
| int verbose = 1, | ||
| NDArray sample_weight = null, |
There was a problem hiding this comment.
It's recommended to put sample_weight the last parameter, to keep its backward compatibility. However if this is exactly the order in python, just keeping it is okay.
There was a problem hiding this comment.
Yes, it follow the order in python.
src/TensorFlowNET.Core/Util/Data.cs
Outdated
| @@ -8,10 +9,10 @@ namespace Tensorflow.Util | |||
| /// </summary> | |||
| public class ValidationDataPack | |||
| { | |||
| public NDArray val_x; | |||
| public OneOf<NDArray, NDArray[]> val_x; | |||
There was a problem hiding this comment.
Is it possible to use NDArray[] alone since NDArray could also be an array of length 1? The purpose is to avoid too many APIs with Oneof exposed to users. If this method is not supposed to be used by our users, I think it's ok.
There was a problem hiding this comment.
The only usage of ValidationPack class is to handle the case that the validation_data parameter in model.fit could be a tuple of two NDArray or three NDArray and deconstruct to two NDArray or three NDArray. So it will not used by users.
There was a problem hiding this comment.
Please consider declaring it as internal, which prevents being called from users.
There was a problem hiding this comment.
Done, really thank you for your advice.
There was a problem hiding this comment.
Is the usage of ValidationPack class limited to internal developers? Therefore I think we should declare the whole class as internal.
There was a problem hiding this comment.
I have tried, but it is used in model.fit as a parameter, which is public, so it will cause an error : Inconsistent accessibility: parameter type 'type' is less accessible than method 'method' if declare it as internal.
fix #1211