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

RESS: Add option to return mixing and unmixing matrices #28

Merged
merged 5 commits into from
Feb 23, 2021

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Projecting RESS components on new observatios of data
2 participants