astro-informatics / s2wav

Differentiable and accelerated wavelet transform on the sphere with JAX
https://astro-informatics.github.io/s2wav/
MIT License
13 stars 0 forks source link

Adding a class of the commonly used parameters #23

Closed JessWhitney closed 1 year ago

JessWhitney commented 1 year ago

I've written a class containing all the parameters, in place of a struct which was used in the original code, from issue #22 . I'm unsure if I've written lines 49 - 52 correctly, so could someone please double check that.

jasonmcewen commented 1 year ago

If we're going to have a dataclass we should use the dataclass decorator and leverage the built in python functionality (https://docs.python.org/3/library/dataclasses.html).

That said, I think we should avoid the object oriented approach and not have a parameter class. We can just use method arguments that are optional and default to typical values, as we do in s2fft.

Any reason to do things differently here @CosmoMatt ?

CosmoMatt commented 1 year ago

@jasonmcewen explicitly passing all variables to each function is certainly a more functional approach, which worked quite nicely for s2fft (where the number of floating parameters is relatively low). Even then the number of parameters being passed around was getting a little cumbersome. Here he have (potentially many) more parameters so this approach may become illegible quickly. I suspect this may be the exact reason why a parameter structure was adopted in the original s2let code.

If we do go down the route of packaging up shared parameters in a nice box that we can pass as a single argument to all the functions then there are a selection of ways we can do this.

The simplest would be using dictionaries, another would be through a dataclass like this. I think doing something like this is a reasonable approach but my only concern would be how this might interplay with JAX later down the line.

jasonmcewen commented 1 year ago

@CosmoMatt @JessWhitney I would like to make the most common cases very easy. For most use cases a user would probably only set a couple of parameters and leave the rest as defaults. I was thinking it would then be quite easy to just have default method arguments. Messing around with a dataclass is slightly annoying if you are really only just changing a small number of parameters.

One advantage a dataclass does have, however, is that it's easy to get a summary later of the parameters used. That would probably be quite useful.

Perhaps we should get inspiration from some other codes. What about TensorFlow? I think they generally just use arguments rather than dataclasses.

JessWhitney commented 1 year ago

@CosmoMatt @jasonmcewen A default case sounds sensible if most of the parameters would remain unchanged for the average user. I'm still new to classes but is it possible to input some default values for each of the variables within the class, thereby still keeping the flexibility to specify different values for the parameters required? Or by doing that does the (data)class method become quite inefficient? I'm happy to alter the parameters in whatever manner we decide is best.