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

major refactoring of conversion related code #175

Merged
merged 27 commits into from
Mar 22, 2017
Merged

Conversation

randy3k
Copy link
Member

@randy3k randy3k commented Mar 17, 2017

towards #138

@randy3k randy3k force-pushed the randy3k/convert branch 4 times, most recently from 7aae989 to 1f0d6e1 Compare March 17, 2017 07:00
@nalimilan
Copy link
Member

support DataTable (with default being DataArrays)

If support for DataTable is added, it should use NullableArrays instead.

@randy3k
Copy link
Member Author

randy3k commented Mar 17, 2017

I meant support DataFrame(R"mtcars") and DataTable(R"mtcars"). Of course, the columns of a DataFrame are DataArrays and the columns of a DataTable are NullableArrays.

@nalimilan
Copy link
Member

Then I don't understand why you wrote that. :-)

@randy3k
Copy link
Member Author

randy3k commented Mar 17, 2017

because I made a stupid typo...

randy3k added 2 commits March 17, 2017 14:30
* pull-request-163:
  Support Nullable Date/DateTime
  Convert Date and DateTime to/from R

# Conflicts:
#   src/convert-default.jl
#   src/convert/base.jl
@randy3k randy3k changed the title WIP: major refactoring of conversion related code major refactoring of conversion related code Mar 21, 2017
@randy3k
Copy link
Member Author

randy3k commented Mar 21, 2017

Will merge soon if there are no other comments.

cc: @simonbyrne @dmbates

CategoricalArrays 0.0.6
NullableArrays 0.1.0
CategoricalArrays 0.1.0
AxisArrays 0.0.6
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you've decided to use AxisArrays rather than NamedArrays? I'm still unsure what's the best approach (e.g. for my package FreqTables). The generality of AxisArray (numeric and categorical axes) is appealing, but the goal of the package does not seem to completely match the R approach to array names. In particular, the printing of names before the data (rather than on each line as in R/NamedArrays) isn't practical for categorical axes.

@mbauman What do you think about this? Are the goals compatible? Could the printing evolve towards what NamedArrays does? i.e. something like this:

4×4 Named Array{Int64,2}
Dim1 ╲ Dim2 │  A   B   C   D
────────────┼───────────────
a           │ 30  20  30  20
b           │ 30  20  30  20
c           │ 20  30  20  30
d           │ 20  30  20  30

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd absolutely like to see better printing of AxisArrays. It's still a young project in terms of the number of hours that have been dedicated to it.

I'm not familiar with any homogenous named array in R, so I'm not entirely sure about the use-case, but the goal of AxisArrays is to provide core support for many disparate domains. Then secondary packages can layer on more domain-specific behaviors.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R named arrays don't have anything really specific. Names are always strings, indexing by name is supported (but only by putting the index in the position corresponding to the dimension). Names are propagated by some operations like element-wise ops and reductions (which isn't supported by AxisArrays yet IIRC). The way they are printed is similar to the NamedArray example above. I was afraid the way AxisArrays are printed was a design decision, but if you would be OK with something more like NamedArray it would be great.

(Then I'm not a fan of a name, I wouldn't have considered dimension names as axes, but I guess that's a minor point. :-)

@randy3k
Copy link
Member Author

randy3k commented Mar 22, 2017

Since the conversion to AxisArrays has to be explicit, i.e. rcopy(AxisArray, foo). It is equally possible to support NamedArrays or AxisArrays or both.

@randy3k randy3k merged commit b6e1d5e into master Mar 22, 2017
@randy3k randy3k deleted the randy3k/convert branch March 22, 2017 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants