Integrated GPFA

[dizcza]

Only gpfa method is integrated here among all other choices in neural_trajectory.
We'll add new functionality once it's implemented by our researchers.

[coveralls]

Coverage increased (+16.8%) to 87.727% when pulling 6101bbb on INM-6:feature/gpfa into d6822d6 on NeuralEnsemble:master.

[alperyeg]

I made a minor review. Mostly concerning documentation. I hope to find some time to have a more detailed look.

[dizcza]

As a side note, gpfa can be implemented as a class. Doing so will reduce a large number of repeated arguments (like verbose, num-of-iters, etc.) that are passed all over the gpfa functionality. I guess, if we agree to this, it'll be a separate PR.

[Junji110]

I found there are number of cases where assert is used to check the type or shape of input variables, but I don't think this is a proper usage of assert; you should rather raise ValueError in such cases.
To my understanding, assert should be used to check the internal logic of the code, but not to check whether users use the code in a proper way.

[dizcza]

I found there are number of cases where assert is used to check the type or shape of input variables, but I don't think this is a proper usage of assert; you should rather raise ValueError in such cases.
To my understanding, assert should be used to check the internal logic of the code, but not to check whether users use the code in a proper way.

Actually yes, ValueError better fits the philosophy of checking the function input, while asserts are used as "I swear this cannot happen". I'll go through input checking asserts and change them to ValueError.

[pep8speaks]

Hello @dizcza! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file elephant/test/test_gpfa.py:

Line 116:52: E231 missing whitespace after ','

Comment last updated at 2019-07-31 13:28:51 UTC

[Junji110]

I've refactored unit tests and added a new test on dimensionality reduction performance of GPFA

[mdenker]

Wow, huge amount of work getting this ported!
Minimal example works for me. I have started a first round of comments where I think a first major issue is to spend some more work on the function documentations, since they are still very difficult for users not familiar with the method. I'd be happy to assist with this as a novice user.

[mdenker]

I am not sure I understand the advantage of using recarray instead of a classic list of dict here? Would

seqs_train['y'][0] # recarrays, trial 0

be preferrable over

seqs_train[0]['y'] # list of dicts, trial 0

from a user perspective?

[dizcza]

I fully agree not using np.recarray. The authors should provide a clear reason why they stick to np.recarray over python-friendly structures. Apart from confusing the users (and probably developers as well), it has several disadvantages during the test setup and future maintenance.

[Junji110]

I also thought to define seqs_train as a list of dicts rather than a recarray, but I found that there are many parts in the code where seqs_train['y'] is subjected to vector operation, which cannot be done quickly with dicts, so I gave up.
We could still define seqs_train as a dict of arrays (of the same length, i.e., # of trials) representing y, x, and so on, but it would require a total rewrite of the whole code.

By the way, for a recarray seqs_train, seqs_train['y'][0] is identical to seqs_train[0]['y'].

[dizcza]

TODO list:

1. Replace np.recarray -> dict: hide np.recarray from the user in GPFA internal private functional, but still operate with dicts as input and output type.
2. Add an example where test data is used as not a part of the training data.
3. Move functions to OOP.