pyMCR icon indicating copy to clipboard operation
pyMCR copied to clipboard

Make the McrAR API more sklearn-like

Open francisco-dlp opened this issue 4 years ago • 3 comments

This makes the API more similar to sklearn's as discussed in https://github.com/hyperspy/hyperspy/pull/2172#issuecomment-625770320.

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.

francisco-dlp avatar May 13 '20 10:05 francisco-dlp

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

ericpre avatar Jun 07 '20 14:06 ericpre

@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 avatar Jun 08 '20 13:06 CCampJr

@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 https://github.com/hyperspy/hyperspy/pull/2172#issuecomment-625770320

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!

CCampJr avatar Jun 08 '20 15:06 CCampJr