[Perl] Sparse feature. by sergeykolychev · Pull Request #9988 · apache/mxnet

sergeykolychev · 2018-03-05T02:05:04Z

Description

Added support for sparse tensors to Perl API

Checklist

Essentials

Passed code style checking (make lint)
Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage:
Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
Code is well-documented:
For user-facing API changes, API doc string has been updated.
For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

Sparse tensors support in Perl API

Comments

@eric-haibin-lin
Please check out this for sanity, it all works afaik but I may have missed something really recent.

sergeykolychev · 2018-03-05T02:11:05Z

@tlby Rob, please take a look, thanks!

tlby

not quite finished reading yet, but I'll wrap up soon. So far, so good though. Nice!

tlby · 2018-03-06T03:41:59Z

perl-package/AI-MXNet/lib/AI/MXNet/Optimizer.pm

+        epsilon => $self->epsilon,
+        rescale_grad => $self->rescale_grad
+    );
+use Data::Dumper;


debugging cruft?

yes, will remove, thanks for noticing.

tlby · 2018-03-06T03:43:56Z

perl-package/AI-MXNet/lib/AI/MXNet/IO.pm

+        (blessed $label and $label->isa('AI::MXNet::NDArray::CSR')))
+            and
+        ($self->last_batch_handle != 'discard')
+    )


"and" and "or" at the same indent depth?

yep, wrong indentation, will fix.

tlby · 2018-03-06T03:55:58Z

perl-package/AI-MXNet/lib/AI/MXNet/NDArray/Sparse.pm

+    $class;
+}
+
+sub not_implemented { confess "Not implemented" }


perhaps "..." is appropriate here? it's kinda standard for declaring abstract functions.

please elaborate

from the perl docs:

When Perl 5.12 or later encounters an ellipsis statement, it parses this without error, but if and when you should actually try to execute it, Perl throws an exception with the text Unimplemented

That seems to be highly aligned the Python source such as https://github.com/apache/incubator-mxnet/blob/0fe04e9d778ac9c55e933ff3f21c4ddf28a4a101/python/mxnet/ndarray/sparse.py#L118 as a pattern for the abstract methods subclasses must provide.

Your code isn't wrong, it's just reimplementing a feature the core language already has.

Thanks man, there's a lot about Perl I still don't know.

tlby · 2018-03-06T04:07:59Z

perl-package/AI-MXNet/lib/AI/MXNet/Base.pm

+}
+
+use B;
+sub hash { hex(B::hash(shift)) }


Careful with this, remember the hash produced will vary from one run to the next.

yeah I know, it's the same with Python's hash() call, so it's a feature.

tlby · 2018-03-06T04:20:56Z

perl-package/AI-MXNetCAPI/mxnet_typemaps.i

+            svs[i] = newSViv((*$1)[i]);
+            sv_2mortal(svs[i]);
+        }
+        myav = av_make(*arg4, svs);


I don't think you can guarantee that "arg4" will be a stable variable name for the other parameter in the function that uses this typemap, so this might blow up at some time in the future, but I'll pester you elsewhere for more details.

I went with this solution because I needed to solve a peculiar problem of making a decision dependent on a particular input state.
You helped me in the past to clean up the swig usage, could you please see if you can come up with a solution later in separate pull that is more stable (I used this technique in another place as well in the past).

okay, sure. I'll get more details and work out a proposal.

eric-haibin-lin · 2018-03-08T07:05:28Z

perl-package/AI-MXNetCAPI/mxnet.i

                              int delay_alloc,
                              int dtype,
                              NDArrayHandle *out);
+/*!


Does this file have to be updated when c_api is changed each time? Is this not automated?

It may look like it could be automated but it can not.
It's not just a copy/paste from the c_api.h, it's a SWIG interface file and the variables names are changed to denote a way of how they should be treated to make c api to communicate with perl.
Another file called mxnet_typemaps.i in the dir has actual glue code that depends on the vars.
It's just another version of 'ctypes' .

eric-haibin-lin · 2018-03-08T07:13:03Z

perl-package/AI-MXNet/t/test_kvstore.t

    }
 }

+sub test_row_sparse_pull


Does it include the test case added a few days ago https://github.com/apache/incubator-mxnet/pull/9887/files ?

yes, I added that test.

eric-haibin-lin · 2018-03-08T07:17:54Z

perl-package/AI-MXNet/lib/AI/MXNet/NDArray/Sparse.pm

+
+=head2 slice
+
+        Returns a sliced view of this array.


The documentation is wrong.. Should be a copy instead of a view. I'll post a PR to fix it (in both perl and python).

* sparse feature. * moved myself to the list of commiters. * changed ImperativeInvoke to ImperativeInvokeEx. * updated MNIFEST * addressed code review * recent python's kvstore sparse changes. * using PDL. * fix docs, trigger tests.

sparse feature.

667b99a

sergeykolychev requested a review from eric-haibin-lin March 5, 2018 02:05

sergeykolychev requested review from marcoabreu and szha as code owners March 5, 2018 02:05

moved myself to the list of commiters.

d002b3a

sergeykolychev mentioned this pull request Mar 5, 2018

CI docker revamp; Add Jetson, Raspberry and CentOS 7 build [MXNET-42][MXNET-43] #9995

Merged

37 tasks

sergeykolychev added 2 commits March 5, 2018 11:14

changed ImperativeInvoke to ImperativeInvokeEx.

a90de86

updated MNIFEST

92b66a1

tlby approved these changes Mar 6, 2018

View reviewed changes

sergeykolychev added 5 commits March 6, 2018 07:42

addressed code review

530aa82

Merge remote-tracking branch 'dev/master' into sparse_feature

fedf682

recent python's kvstore sparse changes.

b413d12

using PDL.

5bb4d9c

fix docs, trigger tests.

e0a63c9

sergeykolychev merged commit 2a9c7d9 into apache:master Mar 6, 2018

eric-haibin-lin reviewed Mar 8, 2018

View reviewed changes

Conversation

sergeykolychev commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Essentials

Changes

Comments

Uh oh!

sergeykolychev commented Mar 5, 2018

Uh oh!

tlby left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergeykolychev Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergeykolychev Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sergeykolychev commented Mar 5, 2018 •

edited

Loading

sergeykolychev Mar 6, 2018 •

edited

Loading

sergeykolychev Mar 6, 2018 •

edited

Loading