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

Split *Dense into *Dense and *MaskedDense #8

Open
2 tasks
chewxy opened this issue Sep 17, 2017 · 5 comments
Open
2 tasks

Split *Dense into *Dense and *MaskedDense #8

chewxy opened this issue Sep 17, 2017 · 5 comments

Comments

@chewxy
Copy link
Member

chewxy commented Sep 17, 2017

From @chewxy on June 27, 2017 2:0

@kabaka0 thoughts? The idea is to as per #128 - the tensor package holds the data structures, and the essential methods (basically implementing Tensor), while things like Add will get moved to tensor/ops

Tasks

  • Write tests to ensure that all the new methods and functions in internal/stdeng can be accomplished using FlatMaskedIterator
  • If yes, then split into *Dense and *MaskedDense

Copied from original issue: gorgonia/gorgonia#131

@chewxy
Copy link
Member Author

chewxy commented Sep 17, 2017

From @kabaka0 on June 28, 2017 21:8

I can see the logic behind the split of tensor - the package is currently quite large, which made it harder to locate methods. As for the split of *Dense, The one problem I can see with having *MaskedDense is that a quite a bit of code would be duplicated. I suppose that we could embed *Dense in *MaskedDense to minimize this. It would not have worked with my initial attempt at Masked (in which the mask was allowed to have an iterator with different stride from the data), but given the decision to drop that, it can be done.

Modifying genlib to support such a change would not need much code editing though, given that there already are checks for whether the object is masked or not.

Can you see any other downside to the embedding?

@chewxy
Copy link
Member Author

chewxy commented Sep 17, 2017

I was thinking precisely embedding

type MA struct {
    *Dense
    mask []bool
...
}

@chewxy
Copy link
Member Author

chewxy commented Sep 17, 2017

(I'll do the heavy lifting.. given I'm working on the sparse and colmajor Tensors) you just provide the struct you have in mind @kabaka0

@chewxy
Copy link
Member Author

chewxy commented Sep 17, 2017

From @kabaka0 on June 28, 2017 21:15

That works.

type MA struct {
    *Dense
    mask []bool
    fillValue interface{}

...
}

@chewxy
Copy link
Member Author

chewxy commented Sep 17, 2017

From @kabaka0 on June 28, 2017 21:16

I am happy to help with it, but if you are actively editing the package then I can leave it to you for now, but feel free to ask me to do stuff whenever needed.

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

No branches or pull requests

1 participant