ACEsuit / Polynomials4ML.jl

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

Faster Chebyshev Basis #40

Closed cortner closed 1 year ago

cortner commented 1 year ago

This PR implements a faster Chebyshev basis since the recursion coefficients don't have to be stored.

@CheukHinHoJerry -- I'd be grateful if you can take a look to see if anything is missing. I will review as well before we merge it into your frule branch which we can hopefully then merge later this week.

cortner commented 1 year ago

@dhan-02 @vai-bhav-m -- while we review the code and tests, would you be willing to add some documentation to record what has been implemented?

cortner commented 1 year ago

On a first quick check of the the code, it all looks great. I have no concerns. @CheukHinHoJerry -- do I remember correctly that the rrules and frules for those basic polynomial embeddings are all generic and no extra work is required for that?

CheukHinHoJerry commented 1 year ago

The tests and the code looks fine to me too. However I think this is inconsistent with the chebyshev_basis constructed from op1d3t since the normalisation is done differently in op1d3t and we should document that carefully to prevent confusion.

For rrule and frules - Yes they are the same. I can also do that easily before we merge frule to main, maybe we can also make a subclass for them like AbstractPolyEmbeddings <: AbstractPoly4MLBasis ?

dhan-02 commented 1 year ago

This PR implements a faster Chebyshev basis since the recursion coefficients don't have to be stored.

@CheukHinHoJerry -- I'd be grateful if you can take a look to see if anything is missing. I will review as well before we merge it into your frule branch which we can hopefully then merge later this week.

@dhan-02 @vai-bhav-m -- while we review the code and tests, would you be willing to add some documentation to record what has been implemented?

Yes will do Professor....

dhan-02 commented 1 year ago

chebyshev_basis constructed from op1d3t

Is there any way to make it consistent with the chebyshev_basis constructed from op1d3t as far as the normalization is concerned?

CheukHinHoJerry commented 1 year ago

I think what you have implemented is the standard Cheb of the first kind, which is fine. But there are codes that build upon the previous basis so we should keep that. I think the best that we can do for now is to document it clearly and keep both of them.

It would be great if @cortner can suggest what we should do.

cortner commented 1 year ago

I agree to just document it for now.

And I like the idea of creating a subclass for the embeddings.

cortner commented 1 year ago

But let's discuss that first please how to do it and how to name it.

CheukHinHoJerry commented 1 year ago

@dhan-02 @vai-bhav-m I have reviewed the docs and it looks good. In addition I think it is better to:

otherwise I think this is ready to merge

cortner commented 1 year ago
CheukHinHoJerry commented 1 year ago

Sorry for confusion. What I mean is just moving the details of ChebBasis to the hyperlinks in docstrings (which is the same as moving it to the comment in the code).

But for some reason, for example when I click on CYlmBasis it direct me to acesuit.github.io/Polynomials4ML.jl/dev/backup/#Polynomials4ML.CYlmBasis, which is in backup.html.

cortner commented 1 year ago

Odd then let's just delete backup.md after the merge

CheukHinHoJerry commented 1 year ago

@cortner I think this is ready now. I didn't clean up the comments of other files since I did that once in frule and I plan to do a final clean up later on. I also added a test to make sure ChebBasis ./ chebyshev_basis is constant.

The conflict is minor and should be fine.

cortner commented 1 year ago

ok, I'll resolve and merge. Any further cleanup in the frule branch. Any future bugfixes etc can be done anytime in further PRs.