usnistgov / pyMCR

pyMCR: Multivariate Curve Resolution for Python
https://pages.nist.gov/pyMCR
Other
80 stars 27 forks source link

Give me little more guide before making PR #6

Closed sshojiro closed 6 years ago

sshojiro commented 6 years ago

I have attempted to make a PR https://github.com/CCampJr/pyMCR/pull/5. However, as you can see, codecov shows some errors. Could you give me little instruction to fix this?

Sorry to disturb you.

CCampJr commented 6 years ago

@sshojiro

Thank you very much for your hard work!

Thank you very, very much for your contribution. I look forward to downloading your code and testing it out.

sshojiro commented 6 years ago

@CCampJr I also appreciate your great contribution so much. I've been thinking to start some research projects involving MCR-ALS. So, this repository will be a great help.

Thank you for your comments. After fixing your feedbacks, I'll make a PR again.

Well, as for the second comment,

In general, I try to avoid using sklearn as this adds a requirement for it to be installed for future users. PCA, for example, can be calculated using SVD from numpy.linalg.svd

I think you should add this description to CONTRIBUTING.rst. Hope that you will do so when you have time. Thank you in advance.

CCampJr commented 6 years ago

@sshojiro

Good point: I will add a note to avoid external libraries unless absolutely necessary to the contributing guide-- thanks!

CCampJr commented 6 years ago

Just added a statement to CONTRIBUTING.rst

CCampJr commented 6 years ago

Thanks for the PR-- closing this issue