exoplanet-dev / jaxoplanet

Astronomical time series analysis with JAX
https://jax.exoplanet.codes
MIT License
41 stars 12 forks source link

`Map()` default initialization of `normalize=None` overrides `Ylm(*, normalize=False)` #157

Closed soichiro-hattori closed 9 months ago

soichiro-hattori commented 9 months ago

Currently, a Ylm(*, normalize=False) instance passed to the y argument of a default initialization of a Map instance is automatically normalized due to the default setting of Map normalize=None (which then turns to normalize=True).

y = Ylm(normalize=False)
smap = Map(y=y)  # default is normalize=None which becomes normalize=True
smap

Output (after merging #156):

Map(
  y=Ylm(data={(0, 0): 1.0}, ell_max=0, diagonal=True),
  inc=1.5707963267948966,
  obl=0.0,
  u=(),
  period=1000000000000000.0,
  amplitude=1.0,
  normalize=True
)

You can prevent normalization by passing normalize=False to the Map initialization but that seems a bit redundant.

I'd propose

  1. Setting normalize=False in the default setting for the Map initialization, or
  2. Explicitly letting the user know the passed Ylm object will be normalized, or
  3. Removing the normalize argument in Ylm.

I could see how you might want to support normalization in both of these objects, but I think it's better if we don't override a user specification by default!

@dfm, @lgrcia

lgrcia commented 9 months ago

That's a bit inconsistent, you're right @soichiro-hattori. I think removing the normalize attribute from Ylm would be the right approach. I would add a normalized method in case that's useful.