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

WIP: Adjustments for common Table interface #55

Closed
wants to merge 1 commit into from

Conversation

andreasnoack
Copy link
Member

This is a tiny one but there might be more changes that I'd like to propose in order to get a common interface for tables that StatsModels can use. The change here is consistent with the behavior in NullableArrays and it might help in the construction of contrasts. I don't think there should be any NullableArrays specific code in StatsModels and the price would then have to be that the ContrastsMatrix has Nullables in it but I guess that is in line with the idea of propagating nullables.

  • Return Array{Nullable{T}} instead of NullableArray{T} in unique

@codecov-io
Copy link

codecov-io commented Feb 24, 2017

Codecov Report

Merging #55 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master      #55   +/-   ##
=======================================
  Coverage   90.89%   90.89%           
=======================================
  Files           9        9           
  Lines         494      494           
=======================================
  Hits          449      449           
  Misses         45       45
Impacted Files Coverage Δ
src/nullablearray.jl 71.79% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 388575a...69ca30f. Read the comment docs.

@coveralls
Copy link

coveralls commented Feb 24, 2017

Coverage Status

Coverage remained the same at 90.891% when pulling 69ca30f on andreasnoack:master into 388575a on JuliaData:master.

@nalimilan
Copy link
Member

Why not. Performance isn't typically an issue with such short vectors.

There's another occurrence to remove in src/subarray.jl. Then the only remaining use of NullableArrays (except for tests, which is OK) is for the optimized version of fill_refs!, used by cut. I guess it can be good enough for now to use the generic AbstractArray{<:Nullable} API, which shouldn't be that slow, and remove the specialized version and the dependency on NullableArray altogether.

@nalimilan
Copy link
Member

This has been fixed by the move to Nulls.

@nalimilan nalimilan closed this Sep 19, 2017
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

Successfully merging this pull request may close these issues.

4 participants