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

Make the McrAR API more sklearn-like #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

francisco-dlp
Copy link

This makes the API more similar to sklearn's as discussed in hyperspy/hyperspy#2172 (comment).

The most drastic change to the API is moving most of the arguments of fit() to __init__(). Alternatively, it would be possible to set those arguments both in __init__() and fit(), with the ones in fit overwriting the attributes given in __init__. That would preserve the API, but I think that it risks confusing the users.

It may be better to implement a fit_transform method instead of adding the transform() method as I have done, since in MCR there is no need for a final transform step.

Finally in this PR there are a couple of changes that have nothing to do with sklearn, but makes integration with hyperspy a lot easier. In particular, the automatic unfolding of the C and ST matrices and the internal call to np.asanyarray.

I haven't adapted the tests, so they'll fail. I'll do it if I get positive feedback.

@ericpre
Copy link
Contributor

ericpre commented Jun 7, 2020

@CCampJr, any change you can have a look at this PR?

@CCampJr
Copy link
Collaborator

CCampJr commented Jun 8, 2020

@ericpre Thanks for the ping: I'll look at it more today. I definitely won't be making changes that affect my current user base, so I'm working to just enable its use in HS.

@CCampJr
Copy link
Collaborator

CCampJr commented Jun 8, 2020

@francisco-dlp @ericpre @AndrewHerzing @jat255 255

See PR #33

  • I wasn't sure if opening a PR was the usual way of counter-offering a PR

Per hyperspy/hyperspy#2172 (comment)

from pymcr.mcr import McrAR
s.decomposition(True)
s.blind_source_separation(3, algorithm="orthomax")
s.decomposition(algorithm=McrAR(fit_kwargs={'ST':s.get_bss_factors()})
s.plot_decomposition_results()

I'm hoping this is an acceptable compromise.

Thanks again for the efforts!

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.

3 participants