Some reduction methods don't error on wrong dimension names

[JamesWrigley]

MWE:

# Does not error
mean(rand(X(10)); dims=:foo)

# Does not error
sum(rand(X(10)); dims=:foo)

# Does error with 'ArgumentError: (X,) dims were not found in object'
std(rand(X(10)); dims=:foo)

I did not audit all of the methods to check their behaviour. I think DD should always throw an error if an invalid dimension name is passed.

[tiemvanderdeure]

Hm thought I fixed this in #1116. But maybe I forgot some methods?

The correct behaviour would be not erroring (imo), since (sum(rand(10); dims = 2) also works.

[JamesWrigley]

Uhhh, that is an enormous footgun 😨 Can we at least make it optional, with e.g. a package-level boolean variable?

[tiemvanderdeure]

Not sure if I understand what the problem is exactly?

We just overwrite the methods, so any other packages are free to do whatever they want.

If your dimarray actually has the dimension you can just use hasdim. What is your use case that makes it an enormous problem?

[JamesWrigley]

Sorry, by 'package-level' I was referring to DD itself, e.g. DD.error_on_missing_dims = true.

The fact that DimArray methods by design don't validate dimension names and will silently do nothing if the wrong one is passed is a bit crazy to me TBH (same for the Array method example you gave). I ran into this in the wild when I made a typo of :x instead of :X and spent some time figuring out the issue.

What was the motivation for allowing it?

[tiemvanderdeure]

What was the motivation for allowing it?

Just consistency with base, where reduction along a dimension not in the array also just returns the array.

If your dimension is X then you can just write dims = X instead of dims = :X, which will probably save you some typos.

[JamesWrigley]

Are there cases where that behaviour is actually useful? In practice I suspect that it will just lead to more bugs. I could try making a PR to revert back to throwing an error if you agree, or add a boolean to the package to make it optional.

[rafaqz]

@JamesWrigley probably good to remember that the surface area of methods this package touches is huge, and not perfectly defined in base. And its really a part time project of a few people.

Many behaviors are literally just the outcome of specific combinations of methods like dims and their internal logic. Some are from good design choices. Some from questionable choices.

When you have ideas like this, the thing to do is make a comprehensive clear case for how you want things to work with examples, and look at possible downsides too.

Contributors here like Seth Axen do that all the time, and get a lot of control over how the package works. Because its so easy to see the scope benefits of the changes they want to make.

Otherwise we will tend to being conservative as you are talking about breaking changes, and given the growing user base here we get angry issues for making those.

[JamesWrigley]

Sure, that is absolutely a reasonable development approach. But I don't think I agree that it was followed for this change. In the linked PR I would not say there's a comprehensive case for the change in behavior and there's no discussion of the possible downsides.

In particular what I'm looking for is a good reason for the behavior apart from consistency with Base, which I don't think justifies the inevitable bugs it will cause.

BTW I do apologize if this is coming across as a bit of an attack on you @tiemvanderdeure , that's not at all my intention. I'm very grateful for you and the other maintainers efforts ❤️

[tiemvanderdeure]

Didn't come across as an attack, don't worry!

I do think consistency with Base is a pretty good argument, but I am not necessarily opposed to changing it.

We would just have to think though some more cases, though. For example what should happen if you reduce along some existing and some non-existing dimensions - sum(rand(X(1:10)); dims = (X, :notadim)). What should happen for DimStacks where different layers have different dimensions?

Before #1116 the error message was also super confusing, so I definitely think it was a good addition. But reverting that PR would now be a breaking change...

[JamesWrigley]

For more context on my perspective, to me named dimensions are the core feature of DD. Other things like lookups and selectors and grouping are also important, but they all depend on dimensions. So to me, being able to call a function on a DimArray with a non-existent dimension is a bit like a Dict that doesn't throw a KeyError when indexing with a non-existent key 😛 Maybe there are situations where it'd be useful but having that as the default behaviour would be a bit awful since you'd constantly have to check if a key was present.

Same thing for dimensions with DD. Imagine for example that you have some complicated function that takes in a DimArray and a dimension name to do some computation over and a user accidentally passes in the wrong name. If the function does a reduction they'll probably get a confusing dimension mismatch or broadcasting error from somewhere deep in the function that does not immediately point to them having passed in the wrong name, and DD does not warn anywhere that authors have to validate dimension names so the function author probably didn't think to add an explicit check.

That's why I find the consistency argument unconvincing. I still haven't seen any examples to the contrary so I would argue that Base's behaviour is objectively bad, and we should not copy it (by default).

We would just have to think though some more cases, though. For example what should happen if you reduce along some existing and some non-existing dimensions - sum(rand(X(1:10)); dims = (X, :notadim)). What should happen for DimStacks where different layers have different dimensions?

Before #1116 the error message was also super confusing, so I definitely think it was a good addition. But reverting that PR would now be a breaking change...

To my mind there is not much difference between sum(rand(X(1:10)); dims = :notadim) and sum(rand(X(1:10)); dims = (X, :notadim)), if a dimension doesn't exist then throw an error. Could you give an example of the DimStack case and what the previous behaviour was? I haven't used it much myself.

And yeah, it would be breaking, but it's also a fairly recent change so with luck not many folks are depending on it 🤞

[rafaqz]

Few points. There is no "with luck"! Breaking changes get breaking versions. There is one about to be merged whenever I have a few minutes for this package. After that you will be waiting many months at minimum. (So get in quick)

We also very rarely go against base behaviour even when its bad, because overall consistency is the most important thing to have in a package.

But also you have a point. It may be best if wrong dimension names never work in any context.

To have this change actually happen you will need to show in code all the methods you want changed. Just making a PR with the changes may be easiest then the scope is very clear.

[JamesWrigley]

Few points. There is no "with luck"! Breaking changes get breaking versions.

Sure, I wasn't implying that it doesn't require a breaking version but rather that the amount of code that would break would be minimal.

To have this change actually happen you will need to show in code all the methods you want changed. Just making a PR with the changes may be easiest then the scope is very clear.

Ok, I'll do that. Do you want to make it optional so folks can opt into it?

[JamesWrigley]

Related issues I found:

• sum and friends with dims error on dims that are not in the DimArray #883
• bug?: dropdims errors when passed a Symbol not wrapped in a tuple #1115
• Fix dimnum and reducing functions #909

[tiemvanderdeure]

Do you want to make it optional so folks can opt into it?

IMO we should just settle on the best way to handle this and use that. Another global options would just add confusion for little benefit.