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

Remove beta warning lazy signal #3282

Merged
merged 2 commits into from Dec 19, 2023

Conversation

ericpre
Copy link
Member

@ericpre ericpre commented Dec 17, 2023

The lazy signal API has been used for several years and it is mature enough: it is stable and as of today, there is no reason that there will significant change any time soon in the API! Therefore, I think that it is fair to remove this warning! @francisco-dlp, @CSSFrancis, what do you think?

Copy link

codecov bot commented Dec 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c9f4a53) 80.25% compared to head (e848200) 80.25%.

Additional details and impacted files
@@                 Coverage Diff                 @@
##           RELEASE_next_major    #3282   +/-   ##
===================================================
  Coverage               80.25%   80.25%           
===================================================
  Files                     147      147           
  Lines                   21854    21854           
  Branches                 5139     5139           
===================================================
  Hits                    17538    17538           
  Misses                   3111     3111           
  Partials                 1205     1205           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@CSSFrancis
Copy link
Member

@ericpre that sounds like a great idea. One thing to note is that distributed computing isn't really fully supported and should probably remain in beta.

I thought I made an issue tracker for that but I don't know if I got around to it. As far as I know the samfire and matrix factorization (as well as downstream things like orientation mapping) are what isn't currently supported using lazy processing. In addition most file types aren't supported either (but I'm slowly adding more and more support)

@ericpre
Copy link
Member Author

ericpre commented Dec 18, 2023

Yes, good point, instead of removing the warning, we should update it to be more specific.

@francisco-dlp
Copy link
Member

I agree that the learning features should remain in beta. They still need a lot of polishing and some basic functionality, such as SVD, work lazily in lazy signals, which sounds right, but that is often not what the user needs. In particular, if things haven't changed since I last used the feature, SVD computes the components on demand.

@CSSFrancis
Copy link
Member

Yes, good point, instead of removing the warning, we should update it to be more specific.

That sounds good to me!

  • matrix factorization: interesting, I was expecting that it would work because I thought that this is fairly standard things but I haven't tried and maybe I am being a bit naive!

I agree that the learning features should remain in beta. They still need a lot of polishing and some basic functionality, such as SVD, work lazily in lazy signals, which sounds right, but that is often not what the user needs. In particular, if things haven't changed since I last used the feature, SVD computes the components on demand.

I think you are right. I also think that the NNMF can have some problems with large datasets (if I remember). It needs to be rewritten to use map_blocks rather than the _block_iterator function. It actually wouldn't be too hard but I don't really use it so I've never devoted the time to re-writing it. It probably would be a good thing to do as there really aren't lazy nnmf codes in python. See this surprisingly empty issue :) dask/dask-ml#96.

That code should probably make its way into something like dask-ml.

Copy link
Member

@CSSFrancis CSSFrancis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me!

@ericpre ericpre merged commit 02e7e6a into hyperspy:RELEASE_next_major Dec 19, 2023
24 checks passed
@ericpre ericpre deleted the remove_beta_lazy branch December 19, 2023 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants