Skip to content

Conversation

@kou
Copy link
Member

@kou kou commented Apr 18, 2017

No description provided.

@wesm
Copy link
Member

wesm commented Apr 18, 2017

As a way to reduce build times, have you considered consolidating code either by object type (all types / arrays / tensors in one place) or data type (e.g. all the uint8 things in one header)? One of the downsides of C++, I guess. It's not a problem either way

@kou
Copy link
Member Author

kou commented Apr 19, 2017

OK. I'll choose object type based consolidation. Because it's suitable for organizing documentation.

Arrow GLib uses one class per file because documentation tool Arrow GLib uses (GTK-Doc) uses one document section per file rule.

Should I do the change in this pull request? Or can I work on the change as another pull request? I want to work on the change as another pull request to do one thing in one pull request.

GObject Introspection on Ubuntu 14.04 is 1.40. It doesn't support
"(nullable)". "(nullable)" was added since 1.42.
@kou kou force-pushed the glib-add-tensor branch from 1c05781 to ed949d4 Compare April 19, 2017 05:09
@wesm
Copy link
Member

wesm commented Apr 19, 2017

@kou I see about GTK-Doc, I did not know that requirement. I will defer to your judgment; if you make any changes it's OK to do them in another pull request.

Copy link
Member

@wesm wesm left a comment

Choose a reason for hiding this comment

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

+1.

So one thing I'm wondering, and want your opinion. I am not sure if having all of the TYPENAME##Tensor classes is very useful, since they only provide a typed data pointer. We would make only a single Tensor class instead, would that make things easier for you?

@wesm
Copy link
Member

wesm commented Apr 19, 2017

cc @robertnishihara @pcmoritz -- I am considering removing the arrow::Tensor subclasses, but I want to see if you anticipate that causing any problems. I'm going to merge this PR for now

@asfgit asfgit closed this in 0dc6fe8 Apr 19, 2017
@wesm
Copy link
Member

wesm commented Apr 19, 2017

@kou kou deleted the glib-add-tensor branch April 19, 2017 23:09
@kou
Copy link
Member Author

kou commented Apr 20, 2017

It's OK to me that Tensor subclasses are removed.
If Tensor class provides a method that returns one element value like Array::GetValue(), Tensor subclasses are useful. But the current Tensor class doesn't provide it.

@pcmoritz
Copy link
Contributor

@wesm I don't think we need the tensor subclasses, so ARROW-860 is perfectly fine. If people in the future want to write C++ application code to modify typed tensors we can reconsider.

@wesm
Copy link
Member

wesm commented Apr 22, 2017

OK. Sounds ogod

jeffknupp pushed a commit to jeffknupp/arrow that referenced this pull request Jun 3, 2017
…ensor

Author: Kouhei Sutou <kou@clear-code.com>

Closes apache#560 from kou/glib-add-tensor and squashes the following commits:

ed949d4 [Kouhei Sutou] [GLib] Support running tests on Ubuntu 14.04
39d40f0 [Kouhei Sutou] [GLib] Add GArrowTensor, GArrowInt8Tensor and GArrowUInt8Tensor
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.

3 participants