propertynames for SVD respects private argument

[arhik]

Current implementation of propertynames for SVD type returns same set of symbols ignoring private argument.

julia> using LinearAlgebra

julia> r = svd([1 3; 4 5]);

julia> propertynames(r)
(:U, :S, :V, :Vt)

julia> propertynames(r, true)
(:V, :U, :S, :Vt)

This PR respects private argument and returns field names if private is false.

julia> using LinearAlgebra

julia> r = svd([1 3; 4 5]);

julia> propertynames(r)
(:U, :S, :Vt)

julia> propertynames(r, true)
(:V, :U, :S, :Vt)

Fixes #55168

[aplavin]

All of (:U, :S, :V, :Vt) are public (documented) properties of SVD, shouldn't they always be returned from propertynames()?

[dkarrasch]

All of (:U, :S, :V, :Vt) are public (documented) properties of SVD, shouldn't they always be returned from propertynames()?

I think I agree. I double-checked and the typical usage is to have properties that don't correspond to fields, but to objects that need to be computed from field objects. In those cases, one would like to hide the internal fields (as private), and show them only when really requested. Applying this rationale to SVD, all four symbols are non-private properties, in particular covering all fields, so there should be no difference whether private is true or false. It remains to understand the reason for the different order.

Reconsidered the case.

[arhik]

@aplavin @dkarrasch Thanks for checking on this.

It did cross my head about public/private conflict this PR might raise; I pushed this PR anyways in order to bring order difference like @dkarrasch noticed and redundancy in this function to light and have a discussion in worst case, and in the best case I guess I am challenging Vs status and discuss what are potential issues in demoting ( or promoting ) V to private.

I agree with @aplavin and @dkarrasch about the point being raised but I am also not completely convinced about V status as public. Here is my half-hearted lame explanation:

V is computed on demand. I would like to classify it as computed field rather. So It made sense to keep V private. By not including V in propertynames I think we indirectly hint users that it is not a direct access, and that there is an extra mechanism which can incur overhead. We can always get V from de-structure syntax and use multiple times at later points without penalty. That drove this pull request. I should have done better job explaining this.

[dkarrasch]

The fact that something needs to be computed on demand and is not a straightforward reference to a field doesn't exclude it from being a property of the object. Things became clear to me when I saw the implementation for BunchKaufman: there, all properties incur computation, and most if not all fields are private.

[arhik]

V is still a property of the object, private or public. Infact, I have a repository that exploits this pattern. And it is convincing for the use case I have since there are extra checks for edge cases or lifetime considerations etc..,. Here V is simply a transpose in getproperty (that something user can do themselves from Vt). There is no additional checks or other computations like other cases. I see BunchKaufman have extra computations that is not trivial for users to do it themselves or delegations to internal functions user doesn't have to know about, so that is justified and convincing.

Taking BunchKaufman as example:

A = [1 2; 2 3];
S = bunchkaufman(A)

julia> propertynames(S)
(:p, :P, :L, :U, :D)

Here propertynames are all different and are not trivially recoverable from other properties. Hope I am making my point clear.

[arhik]

The fact that something needs to be computed on demand and is not a straightforward reference to a field doesn't exclude it from being a property of the object. Things became clear to me when I saw the implementation for BunchKaufman: there, all properties incur computation, and most if not all fields are private.

@dkarrasch I am taking back direct access part I mentioned above. I updated my comment with strike through. I understand now why you brought that up. But rest of the point I am making still hold I think.

[antoine-levitt]

The fact that it's Vt and not V that is returned by lapack feels very much like an implementation detail. There's not much of a penalty to exposing V since it's just a wrapper and not a copy. So if anything it's Vt that should be made private. It's also very misleading that it's called Vt, since for complex svd it's an adjoint and not a transpose, so personally I would never use Vt for fear of confusion ...

[arhik]

@antoine-levitt That makes sense. I agree with penalty to exposing V is negligible. It's probably statically resolved by JIT.

From users' point of view, I would favour @antoine-levitt 's argument to make Vt to private (allowing users to follow their naming conventions) and from julia svd internal implementation point of view I am in favour of V being private. Looks like there is no easy consensus on V/Vt private/public argument.

@dkarrasch @antoine-levitt Shall we fix order difference at least? Do you agree order difference in propertynames with private being true or false is unnecessarily confusing.

[dkarrasch]

@dkarrasch @antoine-levitt Shall we fix order difference at least? Do you agree order difference in propertynames with private being true or false is unnecessarily confusing.

Yes, let's do that but also restrict it to that.