Closed wangzcl closed 3 weeks ago
Congratulations, I see you've done a lot of work :)
I've been on vacation and haven't been able to look at anything. I'll review it thoroughly tomorrow. Is there anything specific you want me to check? Is it ready for a merge? Or do you want to make more changes (perhaps they could be done in another PR)?
Congratulations, I see you've done a lot of work :)
I've been on vacation and haven't been able to look at anything. I'll review it thoroughly tomorrow. Is there anything specific you want me to check? Is it ready for a merge? Or do you want to make more changes (perhaps they could be done in another PR)?
All the changes I've made are described in the comment above (sorry it is too long ...). If you have any thoughts or concerns, please let me know. It does pass all the tests and is able to infer correct archetypes in my preliminary test, so I think it is ready for a merge (but we don't need to hurry though). I do have a few more new commits, which we can discuss later.
Thanks!
Congratulations, I see you've done a lot of work :) I've been on vacation and haven't been able to look at anything. I'll review it thoroughly tomorrow. Is there anything specific you want me to check? Is it ready for a merge? Or do you want to make more changes (perhaps they could be done in another PR)?
All the changes I've made are described in the comment above (sorry it is too long ...). If you have any thoughts or concerns, please let me know. It does pass all the tests and is able to infer correct archetypes in my preliminary test, so I think it is ready for a merge (but we don't need to hurry though). I do have a few more new commits, which we can discuss later.
Thanks!
Certainly, everything looks perfect to me. Congratulations on all the work you've done. The only question I have is about Cython, just in case it causes issues during compilation on some platforms (though it shouldn't). But if it generates the wheels, there’s no problem.
Things you should complete before merging:
In any case, you're doing a great job. I'm currently finishing my thesis and don't have much time, but if you need help with anything, let me know.
I'll check the github workflow to see if there's anything I can do. Thank you for everthing and good luck with your thesis!
I think this should to the trick. Sorry for keeping you waiting
I think this should to the trick. Sorry for keeping you waiting
Don't worry, I've been offline this week because on Friday, September 27, I defended my thesis and I took a few days off. I'm going to merge :)
I did a lot of edits in my fork for my research project. I added orthogonal PGD, did refactorization to align the API and behavior with sklearn official modules like
NMF
, and speed up pgd methods 5x ~ 10x. The fork woks well on my device. It is a too big commit and there are still some changes may be too radical, but I think these changes are worth discussing at least and I am happy to do so.AA
has to be provided as keyword arguments exceptn_archetypes
, following the practice of sklearn.AA
attributes: +n_archetypes_
(actual number of archetypes after fitting) +n_iter_
(actual number of iteration performed) +rss_
, orreconstruction_error_
available after fitting.AA
's class attribute_parameter_constraints
. Parameters are now saved to attributes as is, and is automatically checked at data fitting (with sklearn's@_fit_context
) instead of instance initialization. For reproducibility,random_state
is saved as is, and a new random generator is created at fitting.init
kwarg. Only strings are accepted, as passing a function to kwargs is uncommon for sklearn estimators.AA._compute_archetypes
andAA._loss
. These computations are implemented more efficiently by using in-place matrix calculation infit_transform
and other functions.nnls
to match what is inAA
: default kwarg name ismax_iter_optimizer
and default const is 100.AA
with the actual code. E.g. sparse matrix support is deleted;sklearn.decomposition.NMF
, though we know all different optimizing methods have to do alternating optimization, we do not constrainA_optimize
andB_optimize
in class methods. Instead, we do alternating optimization manually innnls_fit_transform
andpgd_fit_transform
. To implement a new optimizing method xx:xx_fit_transform(X, A, B, archetypes, *, max_iter, tol, verbose, **kwargs)
, whereA
,B
andarchetypes
are initialized $A_0$, $B0$ and $B{0}X$. This function should return optimized A, B, archetypes, number of iterationsi
, a list of loss, a bool flag of convergence.xx_transform(X, archetypes, *, max_iter, tol, **kwargs)
that returns a matrix A._parameter_constraint
and if-else branches inAA.transform
andAA.fit_transform
:if self. method=="xx": do...
The benefit for doing so:AABase_3
andAA_3
classesAA
.transform
, multiple iterations are performed so that A should sufficiently converge to local minimum.np.vdot(a, b)
instead ofnp.sum(a * b)
.n_samples
is large the latter becomes slower than the former (both are sufficiently optimized).unit_simplex_proj
,Condat 2016 Algorithm) in Cython (and also normalized one in M&H paper,l1_normalize_proj
). These two functions project the input array in place and return None. I renamed M&H method as "pseudo_pgd" as their normalization is not an "authentic" projection ...pyproject.toml
to make it compatible with Cython. Runhatch build
to compile.pyx
to.c
and.so
and build the wheel.AABase
. Base classes may be needed later if we implement variants of AA, but before that we are not really sure what to be put into the base class and what to the subclass.n_inits
kwarg like sklearn.Kmeans, return the best one)