ACEsuit / Polynomials4ML.jl

Polynomials for ML: fast evaluation, batching, differentiation
MIT License
12 stars 5 forks source link

Refactoring and Cleanup of Interface #84

Closed cortner closed 2 months ago

cortner commented 3 months ago

The purpose of this PR started as an experiment moving the entire P4ML package over to using Bumper and WithAlloc and removing all usage of ObjectPools. It now includes

My sense now is that the extra cost of allocations in typical ACE models will cost a moderate % of runtime, somewhere between 30-50% typically. This is more than acceptable for prototyping. For fast final models we can then hand-write the chains to get the best possible performance.

I'm unifying and finalizing the pb/pfwd interface while I do this so I can write as much shared code as possible.

TODO

Leave for future PRs

CheukHinHoJerry commented 3 months ago

Should we also remove anything related to FlexArray?

cortner commented 3 months ago

Yes for sure. Most of it should be gone. I just haven't reintegrated all parts of the codebase yet.

cortner commented 2 months ago

@CheukHinHoJerry -- in the spirit of slimming down the package, how do we feel about making the P4ML layers actual Lux layers rather than wrapping them into Lux layers? No rush to decide, but let's chat?

CheukHinHoJerry commented 2 months ago

I quite like the idea of having P4ML.lux since I can then define a layer wrapper for my basis <: an abstract layer type in P4ML and then use all the functionalities conveniently even in other packages in a clean way so I prefer keeping that on the first thought. But let me mull over it and we can discuss tmr? I plan to come to campus.

cortner commented 2 months ago

Sounds good to me. It might actually make it easier to ensure that functinality doesn't start interfering - and we might be able to re-implement the "release" functionality.

cortner commented 2 months ago

I think I've now done everything I want to do for now. I propose the following:

CheukHinHoJerry commented 2 months ago

Looks good. I don't have any concern with the suggestions either.

cortner commented 2 months ago

@CheukHinHoJerry -- thank you. Would you mind then taking a look at the linear layer and SparseProduct?

cortner commented 2 months ago

And maybe we can work together on a model building demo.

CheukHinHoJerry commented 2 months ago

Yes I will take a look at the linear layer first and then the sparseproduct over the weekend.

CheukHinHoJerry commented 2 months ago

SparseProduct looks fine elsewhere. The functionality now it provides is sufficient. I will have a final clean up now.

cortner commented 2 months ago

Thank you!

cortner commented 2 months ago

WithAlloc.jl is now registered in the general registry, which means we could start thinking about merging this PR and registering the next minor version? @CheukHinHoJerry , what is still missing?

CheukHinHoJerry commented 2 months ago

Sorry for late reply - I had some local changes on cleaning up the interface of SparseProduct that unifies with PooledSparseProduct. Just pushed them and now I think it is ready if the CI passes.

CheukHinHoJerry commented 2 months ago

CI stucks at testing SparseProduct, which probably relates to #87. That doesn't happen for me previously but it suddenly occurs for the last commit.

cortner commented 2 months ago

Ok, I'll take a look and see if I can fix it.

CheukHinHoJerry commented 2 months ago

I think I see why - I will fix it now.

cortner commented 2 months ago

Thank you, Jerry!

cortner commented 2 months ago

So maybe my last question for @DexuanZhou and @CheukHinHoJerry -- do you need laplacians HERE in P4ML or is it enough for you that you can use P4ML layers with Duals and HyperDuals?

CheukHinHoJerry commented 2 months ago

I think it is enough for us just to make evaluation of Duals and HyperDuals work. If there is a feature request or there are more than one projects that needs laplacian we can then consider adding that?

cortner commented 2 months ago

so we are actually ready to merge this? If you confirm, I'll go ahead with it :)

CheukHinHoJerry commented 2 months ago

Yes please I am happy for this to be merged.

cortner commented 2 months ago

Everyone - thank you for your feedback and help with this PR. I've now registereed this as v0.3.0 in the ACE registry (not in General yet since I'm having a problem with sphericart...)

cortner commented 2 months ago

This is now also registered in General