SciRuby / nmatrix

Dense and sparse linear algebra library for Ruby via SciRuby
Other
469 stars 133 forks source link

implemented rank of a matrix via SVD as mentioned in issue #608 #615

Closed pgtgrly closed 6 years ago

pgtgrly commented 6 years ago

I have implemented the function to get rank via SVD as svd_rank() in math.rb. I found an abandoned PR here and updated the code for ShapeError,user-defined tolerance along with the required documentation.

Ref #608.

v0dro commented 6 years ago

Two things:

pgtgrly commented 6 years ago

I will get to it!

translunar commented 6 years ago

It may not be possible for the build to pass all tests. There are some problems with the Travis configuration. If you see any error messages that don't make any sense for this particular problem, you can probably ignore them; if you're unsure, please ask. But yes, Sameer is correct that you need to write tests, and those need to pass. :)

pgtgrly commented 6 years ago

@v0dro @mohawkjohn Greetings, I checked the log for Travis build files and the errors are not related to the code that I wrote. In one case the homebrew:science tap is deprecated, in another a log file is missing and rest require a Y/N prompt.

I have added certain tests to math_spec.rb for svd_rank() based on the examples found here. Please let me know if further tests are needed. I have also included nmatrix/lapacke in spec_helper.rb as svd_rank requires nmatrix-lapacke.

On a side note, I think it should be explicitly stated in the Developer guide that rspec v 2.99 is used as LongRun is not implemented in v3.x

pgtgrly commented 6 years ago

I removed "require nmatrix/lapacke " from spec_helper to complete the builds and put a "rescue NotImplementedError". However, it is leading to a nomethod error in this build (using jruby).

translunar commented 6 years ago

On a side note, I think it should be explicitly stated in the Developer guide that rspec v 2.99 is used as LongRun is not implemented in v3.x.

Where did this come up for you? In the gemspec, NMatrix requires rspec 2.14.

translunar commented 6 years ago

Can you please do a squash of all of these commits when you get a chance? Makes them much easier to review.

pgtgrly commented 6 years ago

Greetings John, I have squashed my commits together and I see that it is much cleaner and easier to read. I will make sure to do this in my future PRs. I apologise for the inconvenience that it caused. I tried to run rspec outside without rake and that gave me the error mentioned here would have been helpful if it was mentioned in the developer's guide because of the Test Driven Development of the project.

Like before, I checked the log for Travis build files and the errors are not related to the code that I wrote. In one case the homebrew:science tap is deprecated, in another a log file is missing and rest require a Y/N prompt.

pgtgrly commented 6 years ago

Greetings @mohawkjohn, I have made the required changes. I request you to please review my PR and let me know if something needs to be changed :)