Skip to content
This repository has been archived by the owner on May 4, 2019. It is now read-only.

[WIP] Clean up codebase for a public 0.1.0 release #56

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

johnmyleswhite
Copy link
Member

This PR will ultimately go through the full codebase and clean up any issues that should block a 0.1.0 release.

Current steps:

  • Move design decision comments out into a separate design doc
  • Add Julia 0.4 style inline documentation to all functions for in-REPL docs
  • Remove 1 redundant constructor for NullableArray by using default arguments
  • Add 2 missing constructors for NullableVector and NullableMatrix

There's a lot more to come, but I want to discuss some issues as I come upon them.

Move comments out into a separate design doc
Add Julia 0.4 style inline documentation for in-REPL docs
Remove redundant constructor
Add missing constructors for NullableVector and NullableMatrix
@johnmyleswhite
Copy link
Member Author

One big change I'd like to make is to remove the constructors labeled as numbers 5 and 6, which allow the use of sentinels to provide quasi-literals for constructing NullableArrays. I think this API reflects a real use case that we need to get right, but I feel that this API will not be the final API. (I somewhat suspect the final API will only become clear if we merge a descendant of NullableArrays into Base.)

Given that we might remove this API in the future, I'd prefer that we not ship it to the public.

@johnmyleswhite
Copy link
Member Author

Is there any way to get rid of the function below? (I remember struggling with this in the past.)

function Base.similar{T <: Nullable}(X::NullableArray, ::Type{T}, dims::Dims)
    NullableArray(eltype(T), dims)
end

I'm troubled by the disconnect between how similar(x, Int, (1, )) and similar(x, Nullable{Int}, (1, )) work when this function is available.

@johnmyleswhite
Copy link
Member Author

Thinking about this more, perhaps the problem is actually the "normal" definition of similar?

@quinnj
Copy link
Member

quinnj commented Aug 24, 2015

Perhaps @mbauman can help the discussion on similar?

@johnmyleswhite
Copy link
Member Author

Relevant previous discussion with @mbauman: JuliaLang/julia#11574

@mbauman
Copy link
Contributor

mbauman commented Aug 24, 2015

Great to see this nearing a release! Nice work @johnmyleswhite and @davidagold!

I still stand by my comments in the linked Julia issue: I think similar can really only accept the entire AbstractArray's eltype in order for it to work in generic programming.

In the intervening time since then, @simonster has introduced the very clever StructsOfArrays.jl package, which is a generalization of NullableArray. For a StructOfArrays{T<:Complex}, they run into a similar issue… but it's a little different because now similar is (sometimes) used for math, too:

julia> A = StructOfArrays(Complex{Int}, 2,2)
2x2 StructsOfArrays.StructOfArrays{Complex{Int64},2,Tuple{Array{Int64,2},Array{Int64,2}}}:
 4628173616+13189372320im  4624036272+13189364128im
 4628173712+13189402880im  4582752352+0im

julia> A*.000001
2x2 StructsOfArrays.StructOfArrays{Complex{Float64},2,Tuple{Array{Float64,2},Array{Float64,2}}}:
 4628.17+13189.4im  4624.04+13189.4im
 4628.17+13189.4im  4582.75+0.0im

While it doesn't happen currently, one could imagine abs or some other operation using similar(A, Float64). (@simonster - I think similar there should probably return a normal Array for non-leaf and bitstypes.)

I think the only reason that it feels weird for NullableArray is that Nullable is already in the type name. At the risk of getting shot… is there any way you could typealias NullableArray{T} StructOfArrays{Nullable{T}} and combine efforts?

@johnmyleswhite
Copy link
Member Author

I'm now totally onboard with your previous stance: similar should always take in the eltype as the argument, not the type parameter. I'm going to try removing all of the similar(x, Float64) calls in the codebase and try to move uniformly to similar(x, Nullable{Float64}). I'll report back here with any pain points I hit.

I'm not opposed to using StructsOfArrays in the future, but I'd like to get this out sooner rather than later. In particular, there's been some talk of moving more of the machinery for NullableArray into Base in a future version (by replacing the entire concept of #undef with Nullable), so I'd be hesitant to be engage in another round of redesign when we may end up doing yet another redesign after that.

@mbauman
Copy link
Contributor

mbauman commented Aug 24, 2015

Glad to hear you're convinced. :-) Perhaps NullableArray could have another name for this operation (similarnullable?), but that might just make things more confused.

Yes, I definitely agree that you shouldn't embark on that redesign now. This is sorely needed now and needs to be as efficient as possible for this specific use-case. Generalizations can happen later and have the possibility of being seamless with the above typealias. Like I said, I'm really excited to see this registered.

@davidagold
Copy link
Contributor

I'm late to the party because I've been on the road and tramping through the Badlands in SD for the past few days. Unfortunately, I expect to be similarly indisposed for the near future, but I'll try to help in this clean up effort as best I can.

I do agree that similar should take the entire eltype of the target AbstractArray, and that we ought to change that definition of similar accordingly. I think it's important in this case that we make clear in the documentation that the behaviors of similar and eltype derive from the specification that for any DataType T{U, N} that has AbstractArray{f(U), g(N)} as its super, the eltype of the former must be f(U). Otherwise folks will probably get confused as to why eltype(A::Array{T}) = T but eltype(X::NullableArray{T}) = Nullable{T}.

I'm also curious as to the reasoning why eltype(X::NullableArray{T}) ought to be Nullable{T} and not just T -- i.e. if there is a definitive norm for the relationship between the type parameter of the AbstractArray{f(T)} supertype and the behavior of the array type in question. (I think it ought to be Nullable{T}, but I'd like to be familiar with the logic, too.) Do we just say that whatever is the type of object that gets returned by getindex must be the eltype of the array? This is getting a bit philosophical perhaps.

@johnmyleswhite
Copy link
Member Author

Enjoy your road trip. I'll keep working on this.

I think we've largely settled on the requirement that eltype(x) = T if and only if getindex(x, I::Scalar) = T, although I might not be thinking of more esoteric abstract arrays.

@davidagold
Copy link
Contributor

@johnmyleswhite As far as the pseudo-literal constructors are concerned: I'm fine to remove them, though I'm hard-pressed at the moment to think of a better way to handle the use case they are designed to treat. Let's check with @quinnj to see if he's still using them in his packages and, if so, what they best way to handle that situation would be.

@davidagold
Copy link
Contributor

Thank you, John! And also my thanks to @mbauman and @IainNZ for helping to push this package out while I'm on the move.

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

Successfully merging this pull request may close these issues.

4 participants