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

get_foreground_model refactor #83

Merged
merged 5 commits into from
Sep 4, 2024
Merged

get_foreground_model refactor #83

merged 5 commits into from
Sep 4, 2024

Conversation

cmbant
Copy link
Collaborator

@cmbant cmbant commented Jul 5, 2024

Last one for the moment.. just tries to simplify the code so that foregrounds are used directly as arrays from model dictionary, rather than introducing new large temporary fg_dict structure, most of which was unused.

Now _get_foreground_model is not used internally; I assume this is intended for plotting etc, in which case probably best renamed to get_foreground_model as public facing.

@codecov-commenter
Copy link

codecov-commenter commented Jul 5, 2024

Codecov Report

Attention: Patch coverage is 71.79487% with 11 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@660e0fa). Learn more about missing BASE report.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #83   +/-   ##
=========================================
  Coverage          ?   83.33%           
=========================================
  Files             ?        3           
  Lines             ?      396           
  Branches          ?        0           
=========================================
  Hits              ?      330           
  Misses            ?       66           
  Partials          ?        0           
Files Coverage Δ
mflike/mflike.py 90.40% <100.00%> (ø)
mflike/theoryforge.py 76.43% <60.71%> (ø)

mflike/theoryforge.py Outdated Show resolved Hide resolved
@xgarrido
Copy link
Collaborator

xgarrido commented Jul 6, 2024

It loooks sensible especially the add of sum_all/term variables. The _get_foreground_model is not only used for plotting purpose since pspipe relies on it to get the foreground model the same way the likelihood does. Since the function declaration does not change, I think we are safe (let's see what @thibautlouis and @adrien-laposta think about it).

@cmbant
Copy link
Collaborator Author

cmbant commented Jul 6, 2024

Btw, this currently seems to do nothing: https://github.com/simonsobs/LAT_MFLike/blob/master/mflike/theoryforge.py#L626

@sgiardie
Copy link
Collaborator

sgiardie commented Jul 6, 2024

Btw, this currently seems to do nothing: https://github.com/simonsobs/LAT_MFLike/blob/master/mflike/theoryforge.py#L626

Yes that's true, it has not been fully developed due to the absence of a systematic template to use. I have actually modified that in a branch for some ACT tests, but I doubt it will end up in the master

@cmbant cmbant merged commit bb7194e into master Sep 4, 2024
11 checks passed
@cmbant cmbant deleted the foregroundfactor branch September 4, 2024 07:56
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.

4 participants