Skip to content

Conversation

@Hororohoruru
Copy link
Contributor

Pull request adding the changes discussed in #27

The changes are very minimal. I tried to respect the style for the docstring and added also a small example of the potential use of the returned eigenvectors.

Please check whether the end of the function (intending to take care of any combinantion of return_maps and return_comps) is OK or maybe there is a cleaner way of doing it.

@codecov
Copy link

codecov bot commented Feb 22, 2021

Codecov Report

Merging #28 (6a3469d) into master (0676e2f) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@ ## master #28 +/- ## ========================================== + Coverage 75.74% 75.75% +0.01%  ========================================== Files 18 18 Lines 2057 2058 +1 ========================================== + Hits 1558 1559 +1  Misses 499 499 
Impacted Files Coverage Δ
meegkit/ress.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0676e2f...6a3469d. Read the comment docs.

@nbara
Copy link
Owner

nbara commented Feb 22, 2021

@Hororohoruru thanks !

I've taken the liberty to try and clarify the maps/comps distinction by renaming them to from_ress / to_ress, and expanding the dosctring accordingly.

There is a breaking change wrt to the previous approach, in that I decided that the from_ress / to_ress matrices should have a dimension corresponding to n_keep (previously they were both square matrices). This makes it super easier to project to/from component space.

What do you think ?

@nbara nbara changed the title RESS: Added the option to return eigenvectors used for RESS RESS: Add option to return mixing and unmixing matrices Feb 22, 2021
@nbara nbara added the enhancement New feature or request label Feb 22, 2021
@nbara nbara linked an issue Feb 22, 2021 that may be closed by this pull request
@Hororohoruru
Copy link
Contributor Author

Hello @nbara ! Looks good to me, I think it looks much more clear now. Thanks a lot!

@nbara nbara merged commit 86098ff into nbara:master Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

2 participants