Skip to content

Make the McrAR API more sklearn-like#32

Open
francisco-dlp wants to merge 1 commit into
usnistgov:masterfrom
francisco-dlp:sklearn_like
Open

Make the McrAR API more sklearn-like#32
francisco-dlp wants to merge 1 commit into
usnistgov:masterfrom
francisco-dlp:sklearn_like

Conversation

@francisco-dlp

Copy link
Copy Markdown

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

ericpre commented Jun 7, 2020

Copy link
Copy Markdown
Contributor

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

@CCampJr

CCampJr commented Jun 8, 2020

Copy link
Copy Markdown
Collaborator

@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

CCampJr commented Jun 8, 2020

Copy link
Copy Markdown
Collaborator

@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