search

ACEsuit / Polynomials4ML.jl

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

Sparseproduct ed, ed2 and ProductBasis #36

Closed CheukHinHoJerry closed 1 year ago

CheukHinHoJerry commented 1 year ago

This branch implements:

Some of the functionalities are not complete but it is sufficient for bf in ACEpsi. @DexuanZhou and I are cloning this branch and dev it locally. No urgent merging it before everything is done nicely I think.

cc @cortner

cortner commented 1 year ago

Hi guys - thank you for this PR. I've started reviewing. My first big comment is this : your implementation of evaluate_ed and evaluate_ed2 doesn't correspond to how those functions are specified. What you implemented there are frule implementations if I'm not mistake???. I actually think these are really important and I'm very keen to have them, but we need to find a different name for them.

also, we do need the actual implementations of evaluate_ed and evaluate_ed2 as well.

cortner commented 1 year ago

regarding naming, what about 

_frule_evaluate(...)
_frule_frule_evaluate(...)

the underscore in _frule... indicates that these are private functions. We don't need them to be private, but my thinking is that these are just backends / kernels for an frule method that overloads the ChainRules.frule.

What do you think?

cortner commented 1 year ago

the other point to mention is that it would be good to use the generic interface for the allocating versions of the evaluation functions + the @reqfields macro...

DexuanZhou commented 1 year ago

Hi guys - thank you for this PR. I've started reviewing. My first big comment is this : your implementation of evaluate_ed and evaluate_ed2 doesn't correspond to how those functions are specified. What you implemented there are frule implementations if I'm not mistake???. I actually think these are really important and I'm very keen to have them, but we need to find a different name for them.

also, we do need the actual implementations of evaluate_ed and evaluate_ed2 as well.

Now I realize that the implementation here is indeed not evaluate_ed and evaluate_ed2. Thank you for pointing that out. I will rename these functions and provide the actual implementations of evaluate_ed and evaluate_ed2.

DexuanZhou commented 1 year ago

regarding naming, what about

_frule_evaluate(...)
_frule_frule_evaluate(...)

the underscore in _frule... indicates that these are private functions. We don't need them to be private, but my thinking is that these are just backends / kernels for an frule method that overloads the ChainRules.frule.

What do you think?

These names sound great. I will double-check the existing code to confirm if the functions were originally intended to be frule implementations, and then I will rename them as you suggested!

CheukHinHoJerry commented 1 year ago

the other point to mention is that it would be good to use the generic interface for the allocating versions of the evaluation functions + the @reqfields macro...

I will coordinate with @DexuanZhou and modify the new functions correspondingly.

cortner commented 1 year ago

I will double-check the existing code to confirm if the functions were originally intended to be frule implementations,

Yes, please double-check this, I am just guessing :)

cortner commented 1 year ago

Thinking again about renaming those functions. Since we call the rrules a pullback we should maybe call the frules a pushforward. or at least treat them consistently. Because these codes are not true f/rrules as they only do the differentiation w.r.t. the input I probable prefer the following: 

_pbkX_evaluate(...)
_fwdX_evaluate(...)

or some variant on this: i.e. pullback and pushforward w.r.t. to the input variable X only. Please suggest alternative names

cortner commented 1 year ago

close this since it is included in #39