replace DictTable with DictColumnTable #59
Conversation
|
@ablaom a review would be appreciated. |
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## dev #59 +/- ##
==========================================
- Coverage 92.94% 92.73% -0.22%
==========================================
Files 4 4
Lines 397 399 +2
==========================================
+ Hits 369 370 +1
- Misses 28 29 +1
|
| StatsBase = "^0.33, 0.34" | ||
| Tables = "^1.2" | ||
| julia = "1.6" | ||
| julia = "1.3" |
There was a problem hiding this comment.
Why are we reverting to Julia 1.3 support?
| return dict_table | ||
| cols = Tables.dictcolumntable(target_table) | ||
| colnames = Tables.columnnames(cols) | ||
| dict = OrderedDict{Symbol, AbstractVector}( |
There was a problem hiding this comment.
Re AbstractVector, remind me, why cannot we not have a concrete type here?
| dict = OrderedDict{Symbol, AbstractVector}( | ||
| nm => func(weights, Tables.getcolumn(cols, nm), idxsvec) for nm in colnames | ||
| ) | ||
| dict_table = Tables.DictColumnTable(Tables.Schema(colnames, eltype.(values(dict))), dict) |
There was a problem hiding this comment.
Not sure DictColumnTable constructor is strictly public. Maybe we should be using Tables.dictcolumntable instead?
There was a problem hiding this comment.
The object DictColumnTable is public. So I don't see any harm in using the constructor.
The other alternative is to create my own table as Tables.dictcolumntable isn't a constructor, it a utility method that converts other tables into a DictColumnTable.
There was a problem hiding this comment.
I think it safest to construct your own table and use dictcolumntable.
|
|
||
| function univariate_table(::Type{T}, weights, target_table, idxsvec) where {T} | ||
| table = if T <: DictTable | ||
| table = if T <: DictColumnTable |
There was a problem hiding this comment.
This is fine, but maybe type dispatch instead of if...else...end is more usual?
| * `output_type::Type{<:MultiUnivariateFinite}=DictTable` : One of | ||
| (`ColumnTable`, `DictTable`). The type of table type to use for predictions. | ||
| * `output_type::Type{<:MultiUnivariateFinite}=DictColumnTable` : One of |
There was a problem hiding this comment.
In docs I recommend you qualify DictColumnTable as Tables.DictColumnTable or otherwise explain that this type is owned by 3rd party package.
3 participants
DictTableare just the juliaDicts which doesn't preserve ordering.This PR replaces
DictTablewithDictColumnTablewhich preseve key ordering.