nolearn icon indicating copy to clipboard operation
nolearn copied to clipboard

SliceDict, a sliceable dictionary

Open BenjaminBossan opened this issue 9 years ago • 3 comments
trafficstars

Only basic implementation yet, not finished.

Idea: When wrapping dict input in a SliceDict, we could hopefully the reduce number of if isinstance(X, dict) ... else ... but probably not to 0.

Open points:

  • what to do when slice with int
  • should there be a shape attribute? If so, should it only return (shape[0],)?
  • would it break a lot of user code?

Would solve this issue.

BenjaminBossan avatar Nov 13 '16 17:11 BenjaminBossan

This looks great!

I'm not sure what to do with the int argument to __getitem__ either. Does it make sense to just pass it on and do the same thing as with slices? Does SliceDict support using scalars as values at all?

Should we, as part of this same PR, also concert dicts on the way in (fit, predict; any more?) to SliceDict as well and emit a warning?

dnouri avatar Nov 14 '16 22:11 dnouri

Does it make sense to just pass it on and do the same thing as with slices?

The problem here is the following: Say you have two values, np.zeros(10) and np.zeros((10, 5)). If you slice by int, you get back a scalar and an np.zeros(5). That would not work well with SliceDict, since they don't have equal shape[0].

We could implicitly shape back to the original dimensionality, but that would be an unexpected behavior differing from np.array.

My suggestion with progress would be to see where it's possible to replace dict in nolearn code, where not, and where more adaptations are necessary.

BenjaminBossan avatar Nov 15 '16 19:11 BenjaminBossan

We could implicitly shape back to the original dimensionality, but that would be an unexpected behavior differing from np.array.

OK, let's raise a ValueError then?

My suggestion with progress would be to see where it's possible to replace dict in nolearn code, where not, and where more adaptations are necessary.

Sounds good. Do you want to add implicit conversions + warnings to those methods in this PR? There's a few tests that will help, though they probably won't catch all the corner cases.

For the check before the implicit conversion we probably want to do something like if type(X) is dict instead of isinstance, since the latter would also catch subclasses, that is, SliceDict itself.

dnouri avatar Nov 16 '16 05:11 dnouri