0.6/0.7 compat & various minor fixes #143

quinnj · 2018-01-06T07:54:15Z

No description provided.

…ing sure CSV.write respects writing Date & DateTime differently

alyst · 2018-01-06T13:36:21Z

src/Source.jl

@@ -86,7 +84,7 @@ function Source(;fullpath::Union{AbstractString,IO}="",
    datarow = datarow == -1 ? (isa(header, Vector) ? 0 : last(header)) + 1 : datarow # by default, data starts on line after header
    rows = fs == 0 ? -1 : max(-1, rows - datarow + 1 - footerskip) # rows now equals the actual number of rows in the dataset

-    # figure out # of columns and header, either an Integer, Range, or Vector{String}
+    # figure out # of columns and header, either an Integer,  AbstractRange, or Vector{String}


alyst · 2018-01-06T13:37:31Z

src/Source.jl

@@ -239,7 +239,7 @@ Keyword Arguments:
 * `decimal::Union{Char,UInt8}`: character to recognize as the decimal point in a float number, e.g. `3.14` or `3,14`; default `'.'`
 * `truestring`: string to represent `true::Bool` values in a csv file; default `"true"`. Note that `truestring` and `falsestring` cannot start with the same character.
 * `falsestring`: string to represent `false::Bool` values in a csv file; default `"false"`
-* `header`: column names can be provided manually as a complete Vector{String}, or as an Int/Range which indicates the row/rows that contain the column names
+* `header`: column names can be provided manually as a complete Vector{String}, or as an Int/ AbstractRange which indicates the row/rows that contain the column names


alyst · 2018-01-06T13:45:01Z

src/TransposedSource.jl

@@ -233,6 +234,9 @@ function TransposedSource(;fullpath::Union{AbstractString,IO}="",
            columntypes[c] = typ
        end
    end
+    if !weakrefstrings
+        columntypes = [T <: WeakRefString ? String : T for T in columntypes]


Since T could be e.g. Union{Missing, WeakRefString{UInt8}} (and WeakRefStringArray now supports that) the code here should do the similar thing as a little bit above when converting to categorical.

The same thing should be done for Source.
There's also #138 to consolidate column type detection code.

alyst · 2018-01-06T13:56:30Z

src/Source.jl

-Note by default, "string" or text columns will be parsed as the [`WeakRefString`](https://github.com/quinnj/WeakRefStrings.jl) type. This is a custom type that only stores a pointer to the actual byte data + the number of bytes.
-To convert a `String` to a standard Julia string type, just call `string(::WeakRefString)`, this also works on an entire column.
-Oftentimes, however, it can be convenient to work with `WeakRefStrings` depending on the ultimate use, such as transfering the data directly to another system and avoiding all the intermediate copying.
+* `weakrefstrings::Bool=true`: whether WeakRefStrings should be used internally to speed up file parsing; can only be `=true` for Sinks that support WeakRefStringArrays; note that regular Strings are returned from WeakRefStringArray; WeakRefStrings are only used internally.


"WeakRefStringArrays" is not a proper type name, but maybe for clarity it's possible to adjust the sentence a bit, so that it uses "WeakRefString", "String" etc, e.g

whether to use [`WeakRefStrings`](https://github.com/quinnj/WeakRefStrings.jl) package to speed up file parsing; can only be `=true` for the `Sink` objects that support `WeakRefStringArray` columns. Note that `WeakRefStringArray` still returns regular `String` elements.

codecov · 2018-01-06T15:35:56Z

Codecov Report

❗ No coverage uploaded for pull request base (master@8fcce46). Click here to learn what that means.
The diff coverage is 87.5%.

@@            Coverage Diff            @@
##             master     #143   +/-   ##
=========================================
  Coverage          ?   86.76%           
=========================================
  Files             ?        8           
  Lines             ?      869           
  Branches          ?        0           
=========================================
  Hits              ?      754           
  Misses            ?      115           
  Partials          ?        0

Impacted Files	Coverage Δ
src/validate.jl	`85.71% <ø> (ø)`
src/float.jl	`85.27% <ø> (ø)`
src/CSV.jl	`100% <100%> (ø)`
src/parsefields.jl	`90.68% <100%> (ø)`
src/Sink.jl	`85.36% <100%> (ø)`
src/io.jl	`91.76% <100%> (ø)`
src/TransposedSource.jl	`67.74% <71.42%> (ø)`
src/Source.jl	`96.69% <83.33%> (ø)`

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 8fcce46...ebfe12d. Read the comment docs.

alyst · 2018-01-06T22:29:47Z

src/CSV.jl

@@ -38,6 +38,9 @@ end
    return byte
 end

+substitute(::Type{Union{T, Missing}}, ::Type{T1}) where {T, T1} = Union{T1, Missing}


Actually, that's the kind of function that I wanted to have in Missings/0.7 Base, because it's implicitly used in several places at JuliaData (cc @nalimilan).
Maybe it should be called substitute_type?

In Base the convention seems to be type*, e.g. typeintersect and typejoin (we have discussed adding typesubtract). So why not typesubstitute (but of course it's much less general than the others).

It's true, for the Base it should be more specific. substitute_nonmissing_type? nonmissingtype_substitute? Don't have a good variant that starts with type

I'm not sure we'll be able to include all of these helper functions in Base, so for now it could be just Missings.typesubstitute or something like that.

alyst · 2018-01-06T22:32:01Z

src/CSV.jl

@@ -38,6 +38,9 @@ end
    return byte
 end

+substitute(::Type{Union{T, Missing}}, ::Type{T1}) where {T, T1} = Union{T1, Missing}
+substitute(::Type{T}, ::Type{T1}) where {T, T1} = T1


I guess there also should be

substitute(::Type{Missing}, ::Type{T1}) where {T1} = Missing

rule.

alyst · 2018-01-06T22:34:02Z

src/Source.jl

@@ -179,7 +175,7 @@ function Source(;fullpath::Union{AbstractString,IO}="",
        end
    end
    if !weakrefstrings
-        columntypes = [T <: WeakRefString ? String : T for T in columntypes]
+        columntypes = [Missings.T(T) <: WeakRefString ? substitute(T, String) : T for T in columntypes]


That's where substitute(Missing, T) rule is required, because Missings.T(Missing) == Union{} <: WeakRefString.

alyst · 2018-01-06T22:36:33Z

src/Source.jl

-                    if T >: Missing
-                        columntypes[i] = Union{columntypes[i], Missing}
-                    end
+                if length(levels[i]) / sum(values(levels[i])) < .67 && Missings.T(T) <: WeakRefString


T !== Missing check was here to exclude all-missing columns from the conversion to categorical.

quinnj added 3 commits January 6, 2018 00:20

Get all tests passing on 0.6/0.7; mainly compat changes

23c02e2

Fix #137 by cleaning up default dateformat argument (nothing) and mak…

55e8fcf

…ing sure CSV.write respects writing Date & DateTime differently

Fix #130 by providing more docs for CSV.validate

4507320

alyst reviewed Jan 6, 2018

View reviewed changes

quinnj added 3 commits January 6, 2018 07:58

Remove extra ws

f8e5cc3

Update weakrefstrings docs

fd104d4

Fix up type substituting code

14c9e16

alyst reviewed Jan 6, 2018

View reviewed changes

quinnj added 2 commits January 6, 2018 20:14

Fixup type substituting and windows test failures

b974198

Fix 32-bit test failures

ebfe12d

quinnj merged commit 127041e into master Jan 8, 2018

quinnj deleted the jq/fixes branch January 8, 2018 17:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

0.6/0.7 compat & various minor fixes #143

0.6/0.7 compat & various minor fixes #143

quinnj commented Jan 6, 2018

alyst Jan 6, 2018

quinnj Jan 6, 2018

alyst Jan 6, 2018

quinnj Jan 6, 2018

alyst Jan 6, 2018

alyst Jan 6, 2018

quinnj Jan 6, 2018

alyst Jan 6, 2018

quinnj Jan 6, 2018

codecov bot commented Jan 6, 2018 •

edited

Loading

alyst Jan 6, 2018

nalimilan Jan 7, 2018

alyst Jan 8, 2018

nalimilan Jan 8, 2018

alyst Jan 6, 2018

alyst Jan 6, 2018

alyst Jan 6, 2018

0.6/0.7 compat & various minor fixes #143

0.6/0.7 compat & various minor fixes #143

Conversation

quinnj commented Jan 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 6, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 6, 2018 •

edited

Loading