Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft single-mode MFLikes #88

Merged
merged 32 commits into from
Aug 27, 2024
Merged

Draft single-mode MFLikes #88

merged 32 commits into from
Aug 27, 2024

Conversation

ggalloni
Copy link
Contributor

@ggalloni ggalloni commented Aug 7, 2024

Draft PR for single-mode MFLikes.

This is essentially doing what @xgarrido proposed in #3.
Is there something else to do for this?

@ggalloni ggalloni mentioned this pull request Aug 7, 2024
@cmbant
Copy link
Collaborator

cmbant commented Aug 7, 2024

I think it depends how much we want to tidy up the parameter dependence. I think this sets all the same parameters for all the components? (so e.g. the EE likelihood still depends on the fixed-default T calibration param; but more importantly, also probably by default sampling a_kSZ that it is actually unused)

@cmbant
Copy link
Collaborator

cmbant commented Aug 7, 2024

Btw you can inherit parameters/yaml, so e.g. do not need to duplicate most of the yaml content - can have once in base class and then just define "defaults: polarizations: " etc as needed in derived classes (either in yaml or as inherited class attributes).

Can also use imports in yamls, e.g. see the NPIPE likelihoods in cobaya (params: !defaults ...)

@codecov-commenter
Copy link

codecov-commenter commented Aug 7, 2024

Codecov Report

Attention: Patch coverage is 96.47059% with 3 lines in your changes missing coverage. Please review.

Project coverage is 85.33%. Comparing base (3e669e7) to head (3c58c01).

Files Patch % Lines
mflike/mflike.py 91.66% 2 Missing ⚠️
mflike/foreground.py 98.27% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff               @@
##           restructure      #88      +/-   ##
===============================================
+ Coverage        83.93%   85.33%   +1.40%     
===============================================
  Files                3        3              
  Lines              417      457      +40     
===============================================
+ Hits               350      390      +40     
  Misses              67       67              
Files Coverage Δ
mflike/__init__.py 71.42% <100.00%> (ø)
mflike/foreground.py 89.41% <98.27%> (+1.21%) ⬆️
mflike/mflike.py 82.75% <91.66%> (+1.23%) ⬆️

@ggalloni
Copy link
Contributor Author

ggalloni commented Aug 8, 2024

I fixed the calibration part, so, for instance, EE doesn't need calT to be there, not even fixed. I did this after your suggestion just by modifying the yamls. However, doing the same for the foreground params is far more complex.

The issue is the initialization step, where Foreground doesn't know anything about MFLike, or, at least, I could not make them communicate on that step (after that it can be done). I guess this is what you meant in #3 by "dynamically vary the nuisance parameters", am I right?

The only thing that worked is the workaround I'm pushing after cc27f0f.
Essentially, Foreground and MFLike have all the fg parameters, but each mode-specific likelihood has a supported_params attribute. Then, I force the unsupported params to be fixed to some default value (I am using the ones from the test, but ideally these would be the best-fits of some run I guess).

I know it is not much different than before, but at least this forcefully avoids unsupported params being sampled...

Do you know a way to make components communicate at the initialization step somehow? Or maybe re-trigger some part of the initialization part after that (assignment of parameters, adding a drop tag to the unsupported ones, something like that)?

@cmbant
Copy link
Collaborator

cmbant commented Aug 8, 2024

See comment here simonsobs/SOLikeT#183

@cmbant
Copy link
Collaborator

cmbant commented Aug 14, 2024

Extending this, my attempt to make params match dynamically: #89.
It does require specifying "requested_cls" to the foreground model consistently, but should at least raise an error if it is inconsistent. Or can use TTForeground etc. classes.

@ggalloni
Copy link
Contributor Author

I merged #89 into this PR since I don't have write permissions on the other side.
Then, I added TT+EE, TT+TE, and TE+EE keeping requested_cls on the yamls.

@ggalloni ggalloni marked this pull request as ready for review August 14, 2024 11:57
@cmbant
Copy link
Collaborator

cmbant commented Aug 14, 2024

Looks good to me!

@cmbant cmbant mentioned this pull request Aug 15, 2024
@cmbant cmbant merged commit 3c58c01 into simonsobs:restructure Aug 27, 2024
9 checks passed
@ggalloni ggalloni deleted the TT-TE-EE branch August 27, 2024 09:05
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