Skip to content

Conversation

@dpole
Copy link
Contributor

@dpole dpole commented Apr 19, 2021

  1. Agnostic about the structure of kwargs (which is the same of the
    output derivatives of the models): It can be a nested structure.
  2. To address it, _apply applies a function to the leaf of nested
    dictionaries and lists
  3. Drafted derivatives for CorrelatedFactorizedCrossSpectrum, Join
  4. Derivatives wrt to multipoles are compressed from (ell, ..., ell)
    to (ell, ..., 1). This saves a lot of memory and operations, but
    may lead to mistakes. We have to decide whether it is worthwhile
  5. Relatively minor changes here and there which, however, might break
    some compatibility with external codes
  6. Only the variables should be passed to diff (i.e., those that are
    None in the defaults) and returned in the output dictionary/list

1. Agnostic about the structure of kwargs (which is the same of the
   output derivatives of the models): It can be a nested structure.
2. To address it, _apply applies a function to the leaf of nested
   dictionaries and lists
3. Drafted derivatives for CorrelatedFactorizedCrossSpectrum, Join
4. Derivatives wrt to multipoles are compressed from (ell, ..., ell)
   to (ell, ..., 1). This saves a lot of memory and operations, but
   may lead to mistakes. We have to decide whether it is worthwhile
5. Relatively minor changes here and there which, however, might break
   some compatibility with external codes
6. Only the variables should be passed to diff (i.e., those that are
   None in the defaults) and returned in the output dictionary/list
@dpole dpole requested a review from beringueb April 19, 2021 07:55
@beringueb
Copy link
Collaborator

Minor fixes in frequency.py and power.py. Still need to look at cross.py.
I'm not sure how the changes you've implemented in 4) will propagate when used in FactorizedCrossSpectrum.
At first glance, it seems the implementation in FactorizedCrossSpectrum would depend on the cls used, which I'm not sure is something we want. But I need to have a further look. Will do asap !

@beringueb
Copy link
Collaborator

Had to revert to previous implementation of 4) for now. I have tested everything else and it's working as before. Need to complete the implementation of CorrelateFactorizesCrossSpectrum.
One thing I noticed that might be worth thinking about:
Even when the SED is fixed, a diff method still needs to be implemented for (if we want to use it in FactorizedCrossSpectrum). There is probably a simple way around that (eg. check whether any defaults parameters is None?)

@dpole dpole changed the title Untested changes to diff management Derivatives with respect to parameters May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants