Skip to content

Array implementation#2

Merged
maasencioh merged 4 commits into
masterfrom
pad
Sep 7, 2015
Merged

Array implementation#2
maasencioh merged 4 commits into
masterfrom
pad

Conversation

@maasencioh
Copy link
Copy Markdown
Member

Work on progress

@maasencioh maasencioh changed the title first release Array implementation Sep 6, 2015
@maasencioh
Copy link
Copy Markdown
Member Author

@mljs/collaborators this is only the array implementation, the idea is to extend it to matrix but for now I prefer to merge it now and see the rest later, I'm even thinking to publish in npm in order to use it for Savitsky-Golay and other ML packages

@targos
Copy link
Copy Markdown
Member

targos commented Sep 7, 2015

Can you put the whole output array in the tests ?
Something like:

it('Default test', function () {
        var data = [1, 2, 3, 4];
        var model = padArray(data);
        model.should.eql([0, 1, 2, 3, 4, 0]);
});

Comment thread README.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

unwanted backtick after options

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

may be good to specify that it doesn't change the input array and returns a new one

@targos
Copy link
Copy Markdown
Member

targos commented Sep 7, 2015

made a few comments but looks good !

maasencioh added a commit that referenced this pull request Sep 7, 2015
Array implementation
@maasencioh maasencioh merged commit 95ada11 into master Sep 7, 2015
@targos
Copy link
Copy Markdown
Member

targos commented Sep 7, 2015

👍 thank you!

@targos targos deleted the pad branch September 7, 2015 15:33
@maasencioh
Copy link
Copy Markdown
Member Author

no man thanks to you, the question is should I publish it now to NPM and put it in ML or should I wait until I finish the matrix part?

@lpatiny
Copy link
Copy Markdown
Member

lpatiny commented Sep 7, 2015

I think it should be published without waiting for Matrix

@targos
Copy link
Copy Markdown
Member

targos commented Sep 7, 2015

I agree

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