Consistently adopt column signal convention

[wheeheee]

Currently for filt each column of an array is treated as a separate signal. This isn't the case for mt_cross_power_spectra and mt_coherence, and I'm not sure if there's a reason for that, so this is just a draft. The way they are implemented now means it is probably more performant going down the columns than rows, but it isn't very impactful other than consistency with the rest of the package. The change is kind of breaking.
Also, the documentation for signal dimensions in mt_coherence and mt_coherence! was inconsistent.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.13%. Comparing base (7fc3842) to head (590afda).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #627   +/-   ##
=======================================
  Coverage   98.13%   98.13%           
=======================================
  Files          19       19           
  Lines        3277     3279    +2     
=======================================
+ Hits         3216     3218    +2     
  Misses         61       61           

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[martinholters]

I like the consistency here. I wonder whether we can do anything to ease the transition. I guess it's quite unlikely that the input is a square matrix, so if one were to update DSP to a version after this PR without adjustments to mt_* callees, one would probably see errors due to output dimensions not matching expectations. That is at least better than silently getting different results. But if we could come up with some form of deprecation that would tell people what to do, that would be better. Any ideas?

[wheeheee]

Hm. Well, typically n_channels is quite small compared to n_samples. We could write a heuristic to detect if it's the other way round and emit a warning if so. But if we don't like warnings in functions we could put a warning on first launch/precompile if that's possible. Failing that, a bright red/yellow warning in release notes...

[martinholters]

Hm. Well, typically n_channels is quite small compared to n_samples. We could write a heuristic to detect if it's the other way round and emit a warning if so

Yeah, that would work for the vast majority of cases. Downside: it will be really annoying for any use-case with an unexpected n_channels to n_samples ratio. First you get a relatively-hard-to-track-down bug after updating DSP.jl, and when you've finally fixed it, you get warnings you can't suppress. Is this case sufficiently unlikely that we don't care? (Serious question. I tend to think so, but cannot quite convince myself.)

But if we don't like warnings in functions we could put a warning on first launch/precompile if that's possible. Failing that, a bright red/yellow warning in release notes...

I don't think that's going to work. These warnings will go unnoticed too often.

If we really want to avoid breakage, we should support both argument orders, to be selected with a kwarg, say argument_order, defaulting to :legacy. If it is :legacy, we accept the old order, but emit a deprecation warning asking to switch the argument order and specify argument_order=:consistent. If it is :consistent, we accept the new order. In the release after that, we change the default to argument_order=:consistent and stop supporting :legacy, and eventually, we remove the kwarg completely. But that's probably over-engineered and we should go with the simpler warning outlined above. Or maybe there is some viable middle ground?

[wheeheee]

I still think pretty much all cases will be caught by the heuristic, but I guess the keyword argument method is safer and isn't too much work. However, we probably have to force the depwarn because it's off by default and users won't see it...

[martinholters]

However, we probably have to force the depwarn because it's off by default and users won't see it...

Ah, true, that would just postpone the user's grief... What if we require the kwarg? Well, then one would need to adjust the call-sites anyway and could switch the argument order, so we only would need to support argument_order=:consistent. That would actually be simpler. It's bit like having to pass i_have_fixed_this=true. Seems quite stupid at first glance, but might actually not be that bad. EDIT: To clarify, having to pass argument_order=:consistent might not be that bad; having to pass i_have_fixed_this=true would indeed feel stupid.

[wheeheee]

That also sounds reasonable. But wait, if we instead let the default be say nothing, then let it error by default, we could also provide instructions on how to fix it in the error message. So we would basically require argument_order=:consistent to restore functionality while giving users information to fix it quickly. But then eventually we would have to deprecate that keyword argument, say after 2 breaking releases. Or should it continue as a no-op?
Another alternative: an override for the heuristic warnings?

[martinholters]

That's actually what I've tried to say 😄

Another alternative: an override for the heuristic warnings?

Hm... Yes, that would allow most new code to get away without passing a redundant kwarg. Downside is that a warning is easier missed than an error. But if you suddenly get bad results, you probably look closer, and then notice the warning. So a clear warning and an i_know_what_Im_doing=true kwarg to silence it might be best. Only the probably very few current uses with fewer channels than samples will get bad results without a warning.

[wheeheee]

Yeah, I was thinking something like f(s; argument_order) instead of f(s; argument_order=nothing) at first... Anyway, we might be worrying too much, because there seems to be only one package (NeuroAnalyzer.jl) using these functions. It should be enough to drop them a note.

[wheeheee]

Oh, and NeuroAnalysis.jl. The package names looked similar...

[martinholters]

No need to rush anything here, but I've added it to the 0.9 milestone so we don't forget about it.

[ararslan]

This isn't the case for mt_cross_power_spectra and mt_coherence, and I'm not sure if there's a reason for that

Those came out of work undertaken by folks at Beacon Biosignals and upstreamed here in #401. Beacon uses the convention of rows as channels, largely because it's how Onda.jl is designed.

One option here would be to introduce a keyword argument like channel_dimension or whatever that denotes which axis of the matrix represents channels and have it set to nothing by default, which allows you to emit a deprecation warning when not set explicitly to 1 or 2. You can then also warn when 1 is passed to make clear that the default will be 2 and then eventually remove the keyword argument altogether.