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

JRuby port of NMatrix #558

Merged
merged 168 commits into from
Feb 3, 2017
Merged

Conversation

prasunanand
Copy link
Member

rake spec leads to no failure

…rectory. It gets added in the next commit. nmatrix_test.rb has been deleted permanently. We use existing tests.
…n nmatrix class methods shape, dim , slice ,[] , []= , effective_dimension, is_symmetric, etc.
… subtraction, multiplication and division modified accordingly
@prasunanand prasunanand mentioned this pull request Dec 16, 2016
@prasunanand
Copy link
Member Author

jruby_port: travis script2 for jruby

	jruby_port: travis script3 for jruby

jruby_port: remove jruby env from allowed failures
@prasunanand
Copy link
Member Author

Travis tests for JRuby pass and removed from "allowed failures" :) . Please review.

@wlevine
Copy link

wlevine commented Jan 17, 2017

One concern: the dtype argument is totally ignored. I understand that currently only doubles are supported, but an error should be thrown if I try to create a matrix with an unsupported dtype. The same with stype. You should refuse to create a matrix if someone specifies an stype other than :dense.

@dtype = hash[:dtype]
@stype = hash[:stype]
else
@dtype = :double
Copy link

Choose a reason for hiding this comment

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

This should be :float64 not :double

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. I will correct it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not throwing any error here because a lot of tests will fail in that case. And I assure you that this feature will be properly implemented in next PR.

@wlevine
Copy link

wlevine commented Jan 17, 2017

It seems like rake package is broken on your branch.

tar zxvf commons-math3-3.6.1-bin.tar.gz
mkdir ext/nmatrix_java/vendor/
cp commons-math3-3.6.1/commons-math3-3.6.1.jar ext/nmatrix_java/vendor/

Copy link

Choose a reason for hiding this comment

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

Are all these steps still required? It seems like this is already done by the rake task.

Also is this the standard way of dealing with Java libraries (just downloading them and sticking the jar in a random directory)? It seems a little unfortunate.

Are you planning to get this to work with a simple gem install?

Also, you should probably add a small note to the README, noting the limited features of the JRuby version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are all these steps still required? It seems like this is already done by the rake task.
=> These steps are not done by rake task. And, a rake task can be added to achieve this.

Also is this the standard way of dealing with Java libraries (just downloading them and sticking the jar in a random directory)? It seems a little unfortunate.
=> AFAIK, this is the only way.

Are you planning to get this to work with a simple gem install?
=>Yes, It works with gem install

Also, you should probably add a small note to the README, noting the limited features of the JRuby version.
=>Sure

Copy link
Member Author

@prasunanand prasunanand Jan 20, 2017

Choose a reason for hiding this comment

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

There is some error with json-version. I will correct it. I left it earlier, because there was no error with travis tests. So, I thought that the error was limited to my machine only.

@translunar translunar merged commit f5c87a1 into SciRuby:master Feb 3, 2017
@v0dro
Copy link
Member

v0dro commented Feb 9, 2017

Finally this is merged! Congratulations @prasunanand =)

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.

4 participants