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

Add matrix constructing taking only shape (2 variations) #100

Merged
merged 1 commit into from
Jan 21, 2017

Conversation

bartvanandel
Copy link
Contributor

This pull request introduces a change to the Matrix constructor so it accepts shape alone (without any data). Two variations are supported:

var m = new Matrix(rows, cols);
`var n = new Matrix({ shape: [rows, cols] });

I've also refactored the constructor a bit so all different input variants it supports are now accessible through static Matrix.fromXXX(...) functions. This is in line with the already existing Matrix.fromTypedArray and Matrix.fromArray functions.

To protect from obvious failure I've been so kind to add unit tests for both the constructor and the fromXXX(...) functions.

@mateogianolio
Copy link
Owner

This is awesome! I'm going to need some help reviewing these pull requests, because they are piling up and I don't have that much free time anymore. I'm thinking of adding @Xuefeng-Zhu and you as main contributors so you have access to merging PR's, etc. What do you think?

@bartvanandel
Copy link
Contributor Author

Thanks for the heads-up @mateogianolio ! That sounds nice, although I can't promise for sure how much time I can actually invest. I made those changes because a) I needed them (we're using npm but the app runs in the browser) and b) having constructors like in this PR is pretty convenient for us. But of course being able to contribute to the main project would be nice!

We will need to determine the process of pushing out updates to npm though?

@bartvanandel
Copy link
Contributor Author

Will this PR be merged anytime soon? The extra issue reported by 'codeclimate' is more or less unavoidable when simply extending the if-else construction in the constructor.

Alternatively we could drop the convenience if-else constructor structure and provide only 1 valid set of constructor arguments. All other ways of constructing a Matrix are still possible using the static helper functions. This is the most consistent way of dealing with this problem anyway: supporting a few different ways of constructing in the constructor but others only using static helper functions is less consistent.

@mateogianolio
Copy link
Owner

Will merge and draft a new release tomorrow, apparently I can't merge from my phone if a check has failed. Thank you for this addition!

@bartvanandel
Copy link
Contributor Author

No worries, I'm just keeping an eye out :)

@mateogianolio mateogianolio merged commit f2564e7 into mateogianolio:master Jan 21, 2017
@mateogianolio
Copy link
Owner

I think it would be great to add this functionality to vectors as well.

@bartvanandel bartvanandel deleted the matrixFromShape branch January 26, 2017 21:05
ukrbublik added a commit to ukrbublik/vectorious-plus that referenced this pull request Aug 20, 2017
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.

2 participants