-
Notifications
You must be signed in to change notification settings - Fork 12
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
fixing example file + allowing lmax_theory to set ell range #92
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #92 +/- ##
=======================================
Coverage 85.24% 85.24%
=======================================
Files 3 3
Lines 454 454
=======================================
Hits 387 387
Misses 67 67
|
I also expanded a bit/fixed something in the docs. It does not really relate with the other two fixes but they are all minor things that can be merged together |
I agree that I'm not saying that the previous code is better. As far as I remember, we did it that way due to incompatible choice between the lmax value (around 9000) used when simulating data (data that can be cutted at l=5000 afterward) and the lmax value used when reconstructing parameter with |
Maybe I was a bit sloppy when explaining, we don't have this problem in the current default but it would be a problem if the user decides to set
I think it is still unlikely the user would change the lmax without a valid reason. |
@@ -105,7 +105,7 @@ def initialize(self): | |||
self.log.info("Initialized!") | |||
|
|||
def get_fg_requirements(self): | |||
return {"ells": self.l_bpws, | |||
return {"ells": self.l_bpws[:self.lmax_theory + 1], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm always a bit scared when cutting numpy array since I never know what the first indice is. Basically if self.l_bpws
starts at 2 then I think we have a problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, l_bpws should start from 2 if I remember well. So to have it arriving up to lmax_theory we should put [:self.lmax_theory -1]
I fixed the example file since a likelihood block with
mflike.MFLike
instead ofmflike.TTTEEE
does provide enough default settings needed by mflike.I also allowed
lmax_theory
to setself.ells[-1]
. Before, it was only setting the ell max of the theory Cl, but notself.ells
passed to the Foreground class. Now the ell max is set for both tolmax_theory
, as asked by @itrharrison for some SOLikeT tests.It's also not super useful to have
"Cl": {k: max(c, self.lmax_theory + 1) for k, c in self.lcuts.items()}}
(that I fixed to
"Cl": {k: self.lmax_theory + 1 for k, _ in self.lcuts.items()}}
)If
lmax_theory > self.lcuts
, then it is like in the new setting, if insteadlmax_theory < self.lcuts
, we made a mistake in requesting anlmax_theory
smaller than some of the ells indefault_cuts["scales"]
.