axes(::ReshapedDistribution) should throw, added missing ndims

[stevengj]

As noted on discourse, this method was missing. Also added ndims.

Update: from the discussion below, since d::ReshapedDistribution is not indexable, it seems that the actual problem is that axes(d) is defined at all thanks to the generic fallback in Base. So, this PR is updated to throw a MethodError.

Update: @devmotion preferred not to have axes(d) throw an error, so I removed it.

[devmotion]

AFAIK we never intended to define or use axes for distributions. ReshapedDistributions is not special at all here - we define length (number of elements of the variates) and size (size of the variates) for all distributions but generally distributions shouldn't be something AbstractArray like.

My impression is that the problem is actually the use of axes - which only works with a single argument (unintentionally) since the completely generic definition in Base falls back to size.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.99%. Comparing base (e1340f0) to head (adbfadf).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1892   +/-   ##
=======================================
  Coverage   85.99%   85.99%           
=======================================
  Files         144      144           
  Lines        8666     8667    +1     
=======================================
+ Hits         7452     7453    +1     
  Misses       1214     1214           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[devmotion]

It seems maybe the actual problem was a typo in the Turing model: https://discourse.julialang.org/t/axes-not-working-on-reshaped-array/118820/5?u=devmotion

[stevengj]

If you have size, shouldn't you have axes? Or does it not support indexing? If you don't want axes(a, k), probably axes(a) should be redefined to throw a MethodError to circumvent Base's definition.

Should probably still have ndims?

[devmotion]

I'm not sure, I tend to think it's fine as it is.

Distributions are not arrays and therefore we don't implement the array interface (and hence I think we might also not want to implement ndims). That should also be clear from the docs.

Distributions support length and size, and both of them are not array-specific (eg in Base they are also implemented for Number and AbstractChar). That should also be clear from the docs.

Apparently someone decided to implement axes in a very generic way, for any type and only relying on size. But that doesn't really do any harm - it doesn't error but also it's not mentioned anywhere in the docs, it's not part of the distributions interface, so users shouldn't rely on it.

Anyone could write generic functions that only depend on eg length or size but might not make sense for distributions (there might be more in Base?) - it seems infeasible to implement a MethodError for all these cases?

[stevengj]

It seems odd to implement size and not ndims, and you seemed to be missing an easy generic API to get this information.

But I'm fine with removing axes if that's what you want.

[devmotion]

It seems odd to implement size and not ndims, and you seemed to be missing an easy generic API to get this information.

AFAIK it hasn't been requested yet, so I assume therefore it's not defined. Since the API of distributions already is quite large, I would like to not add to it without clear need.

Note also that size is defined for all distributions (but special in so far as it refers to the size of the variates - whose exact types might not be known - but not the distribution itself), so probably if ndims is added at some point it should be defined more generally.

So I'd suggest closing the PR, now that it has become clear that the initial problem was an incorrectly defined Turing model.