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

Sklearn-like functionality and HyperSpy compatibility #33

Merged
merged 2 commits into from
Aug 27, 2021

Conversation

CCampJr
Copy link
Collaborator

@CCampJr CCampJr commented Jun 8, 2020

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

  • Added a fit_kwargs to McrAR so things such as ST and C will be passed onto the fit method without explicitly having to do so
  • Added fit_transform, which returns the calculated C_ array
  • Added a components_ property to McrAR, which is synonymous with ST_
  • Added tests

I have not yet address the comment below from @francisco-dlp in PR #32

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.

@tjof2
Copy link

tjof2 commented Sep 8, 2020

What's the status of this?

@CCampJr
Copy link
Collaborator Author

CCampJr commented Sep 8, 2020

What's the status of this?

Just responded in the HyperSpy PR thread. Basically, I can get that pushed ASAP once you all are confident it will solve the problem with integration.

Thanks!

@francisco-dlp
Copy link

It looks good except for one small detail that currently raises an error:

>>> s.decomposition(algorithm=McrAR(fit_kwargs={'ST':s.get_bss_factors()}))                
INFO:hyperspy.learn.mva:Performing decomposition analysis
INFO:hyperspy.learn.mva:Undoing data pre-treatments
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-8-b37c44b9db40> in <module>()
----> 1 s.decomposition(algorithm=McrAR(fit_kwargs={'ST':s.get_bss_factors()}))

/home/francisco/Git/hyperspy/hyperspy/learn/mva.py in decomposition(self, normalize_poissonian_noise, algorithm, output_dimension, centre, auto_transpose, navigation_mask, signal_mask, var_array, var_func, reproject, return_info, print_info, svd_solver, copy, **kwargs)
    499             elif is_sklearn_like:
    500                 if hasattr(estim, "fit_transform"):
--> 501                     loadings = estim.fit_transform(data_)
    502                 elif hasattr(estim, "fit") and hasattr(estim, "transform"):
    503                     estim.fit(data_)

/home/francisco/Git/pyMCR/pymcr/mcr.py in fit_transform(self, D, **kwargs)
    572         """
    573 
--> 574         self.fit(D, **kwargs)
    575 
    576         return self.C_

/home/francisco/Git/pyMCR/pymcr/mcr.py in fit(self, D, C, ST, st_fix, c_fix, c_first, verbose, post_iter_fcn, post_half_fcn)
    392                 if self._ismin_err(err_temp):
    393                     self.C_opt_ = 1 * C_temp
--> 394                     self.ST_opt_ = 1 * self.ST_
    395                     self.n_iter_opt = num + 1
    396                     self.n_above_min = 0

TypeError: unsupported operand type(s) for *: 'int' and 'Signal1D'

There is an easy fix for this: hyperspy signals can be multiplied by numbers on the right but not on the left so, to fix it it would suffice to replace things like:

self.C_opt_ = 1 * C_temp

with

self.C_opt_ = C_temp * 1

Is that feasible @CCampJr ?

@ericpre
Copy link
Contributor

ericpre commented Aug 23, 2021

@CCampJr, any change to address @francisco-dlp's comment above? Would it help to send a pull request to https://github.com/CCampJr/pyMCR/tree/sklearn_compat?

@CCampJr CCampJr merged commit 49da2a3 into usnistgov:master Aug 27, 2021
@ericpre
Copy link
Contributor

ericpre commented Sep 1, 2021

Thanks @CCampJr for merging this PR. I have open #39 to fix the issue mentioned by @francisco-dlp above.

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.

4 participants