Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maybe we should flip nested wrapper constructors #121

Open
oxinabox opened this issue Jul 28, 2020 · 5 comments
Open

Maybe we should flip nested wrapper constructors #121

oxinabox opened this issue Jul 28, 2020 · 5 comments

Comments

@oxinabox
Copy link
Member

#120 makes me think that
Maybe we should revisit the decision not to do overloads of
Symmetric(NamedDimsArray(x)) --> NamedDimsArray(Symmetric(x))
and Diagonal(NamedDimsArray(x)) --> NamedDimsArray(Diagonal(x))
etc.
cc @nickrobinson251 @mcabbott

@glennmoy
Copy link
Member

Is/was there already an issue for this?

@nickrobinson251
Copy link
Contributor

nickrobinson251 commented Jul 28, 2020

we discussed this before somewhere, right? might be good to go look back at what the considerations were.
I suspect it's that Symmetric(x) isa Symmetric should always be true (or an error)?

@Tokazama
Copy link
Contributor

I'd just like to note that I'm getting close to reaching feature parity with ImageCore.jl by combining NamedDims.jl and AxisIndices.jl. This one has been a bit of a headache. I know it's easier in the short term to have NamedDimsArray the top most layer, but it ends up changing the behavior of stuff that expects a certain array type following methods like permutedims.

@mcabbott
Copy link
Collaborator

Not sure there was an issue, but at some point I had a PR to make PermutedDimsArray(nda, ...) return a NamedDimsArray, and there was some discussion there, in which some felt strongly that T(x) isa T should always hold. (I have forgotten who though.)

I guess I still think that, in the absence of a lower-case constructor like transpose, the type-name Symmetric plays two roles, and so it's not so weird to overload it.

@rafaqz
Copy link

rafaqz commented Aug 1, 2020

I remember talking about it here somewhere ages ago. DimensionalArray in DimensionalData.jl is always the outer wrapper and I've had no issues with it so far.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants