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

Issue/437/magnification bias #438

Merged
merged 62 commits into from
Nov 19, 2021
Merged

Issue/437/magnification bias #438

merged 62 commits into from
Nov 19, 2021

Conversation

marina-ricci
Copy link
Collaborator

No description provided.

combet and others added 5 commits June 11, 2021 17:01
feat: notebooks to reproduce the figures of the v1.0 paper
* add Paper_v1.0.ipynb

* update Paper_v1.0.ipynb

* update examples/Paper_v1.0.ipynb

* add support/paper_formating.py

* make Paper_v1.0.ipynb use support/paper_formating.py

* improve plots on examples/Paper_v1.0.ipynb

* fix comment on Paper nb

* select case for paper

* improve plot

* rm unused lines

* update text in Paper nb

* reset Example1 to master version

* update to new ver of code

* update gt plot (add gx)

* put paper files in one dir

* New notebook to reproduce Fig4 of the v1.0 paper

* Normalize pdf in the corner plot - need to have the same number of samples for both cases

* Update version on Fig3 to answer Referee 3''s comment of v1.0 paper

* update Fig3_update with different fits

* rm old Fig3

* renamed:    Fig3_update.ipynb -> Fig3.ipynb

* update text on Fig3

* Make nicer mcmc plot for paper

* rename paper notebooks

* update text in gt_and_use_case.ipynb

* Moving new notebooks for v1.0 paper to the proper directory

* Small change to NB to solve issue foreground galaxies

* Fix small bug in mock_data

* Small adjustment to figure

* Small adjustment to figure

* Change legend

* Small update to notebook to fit only the bins with sufficient number of galaxies

* Fix bug with SRD z distribution

* Update gt_and_use_case with galaxy density instead of galaxy number

* Add comment to NB

* Typo in NB

* update text and format in gt_and_use_case.ipynb

* update plot

* Update backend comparison plot to start at higher radii

* Revert to previous range and shade up to 5.e-2 Mpc

* update backend validation tests with optimised CCL integration for the v1.0 paper

* Removed dots from abbreviations (ana. -> ana and opt. -> opt). Just to have one notation and simplify a bit the legend.

* Update __init__.py

Update version number

Co-authored-by: m-aguena <aguena@if.usp.br>
Co-authored-by: Michel Aguena <aguena@lapp.in2p3.fr>
Co-authored-by: Mariana Penna Lima <pennalima@gmail.com>
@coveralls
Copy link

coveralls commented Oct 1, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling db63826 on issue/437/magnification_bias into d97a61f on main.

@marina-ricci
Copy link
Collaborator Author

This issue is also linked to issue #444.

@marina-ricci
Copy link
Collaborator Author

This issue covers the first step to integrate weak lensing magnification observables in CLMM. Here I added the "magnification bias" observable in the theory module.

  • include magnification bias in theory

  • write corresponding unit test

  • write corresponding examples in notebooks

@combet combet linked an issue Oct 26, 2021 that may be closed by this pull request
3 tasks
@m-aguena
Copy link
Collaborator

@marina-ricci it looks like the error is on the value of the magnification bias test now.

@m-aguena m-aguena linked an issue Oct 28, 2021 that may be closed by this pull request
Comment on lines 64 to 65
magnification_bias_from_magnification = np.array(
magnification)**(np.array([alpha]).T - 1)
Copy link
Collaborator

@combet combet Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marina-ricci - This gives a warning RuntimeWarning: invalid value encountered in power whenever attempting to take non-integer power of negative values (of the magnification). This happened when running the notebook for alpha=-0.5, and the corresponding magnification bias is then nan. It's not necessarily an issue, but I'm wondering if this can be made clearer in a comment in the code or warning for the user? But maybe that might be more confusing than helpful... It just got me confused for a bit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what to do. The magnification can only be negative in the strong lensing regime so in practice this is not so problematic. But I'm not sure what should be the exact formula here (returning a complex number, a nan..). I'll investigate.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For negative magnifications, the output will be nan by default.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right - we're not going to make use of a complex number, so I think a nan is fine. I'm more thinking in the line of having an explicit warning for the user, like the one we have if some sources are in front of the cluster...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An explicit warning will be helpful for users. I was confused by the RuntimeWarning.

Copy link
Collaborator Author

@marina-ricci marina-ricci Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll add a simple warning and maybe we can think of a more general one to warn the user when we enter the strong lensing regime.

Copy link
Collaborator Author

@marina-ricci marina-ricci Nov 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@combet , @hsinfan1996 , Ok, it should be done now.

Copy link
Collaborator

@m-aguena m-aguena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make sure the lines are shorter than 100 characters? You can check in the files you changed with the command grep -x '.\{100,\}' my_file.py

@@ -9,12 +9,12 @@
import numpy as np

from . import generic
from . generic import compute_reduced_shear_from_convergence
from . generic import *
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please do not import *

convergence : array_like
Convergence
shear : array_like, float
Shear
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is an extra tab here

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also on lines 23 and 28

@@ -281,7 +283,7 @@ def eval_convergence(self, r_proj, z_cl, z_src):
\kappa = \frac{\Sigma}{\Sigma_{crit}}

or

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra tab

@@ -374,3 +376,34 @@ def _eval_magnification(self, r_proj, z_cl, z_src):
kappa = self.eval_convergence(r_proj, z_cl, z_src)
gamma_t = self.eval_tangential_shear(r_proj, z_cl, z_src)
return 1./((1-kappa)**2-abs(gamma_t)**2)

def eval_magnification_bias(self, r_proj, z_cl, z_src, alpha):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add an validation function? just follow the model of eval_magnification

@@ -2,7 +2,8 @@
CLMModeling abstract class
"""
import numpy as np
from .generic import compute_reduced_shear_from_convergence
import warnings
from .generic import *
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please do not import *

@marina-ricci
Copy link
Collaborator Author

@m-aguena , I think it should be good now!

@hsinfan1996 hsinfan1996 linked an issue Nov 19, 2021 that may be closed by this pull request
Copy link
Collaborator

@m-aguena m-aguena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marina-ricci it looks good for me now.
I would just like @combet to have a final look before merging as I made a couple of changes.

@marina-ricci
Copy link
Collaborator Author

@hsinfan1996 Please be careful because the checks are failing since your commit.

@hsinfan1996
Copy link
Collaborator

@hsinfan1996 Please be careful because the checks are failing since your commit.

I noticed, but I really don't think the check failures were related to my commit.

@combet
Copy link
Collaborator

combet commented Nov 19, 2021

The error appears when installing CLMM during CI (that's a new one...)

error: numpy 1.17.4 is installed but numpy>=1.18 is required by {'astropy'}
Error: Process completed with exit code 1.

@hsinfan1996
Copy link
Collaborator

hsinfan1996 commented Nov 19, 2021

@combet @m-aguena astropy 5.0 is installed during install prereq using pip, since we only require astropy to be >=4. Too new.

@hsinfan1996
Copy link
Collaborator

Need to edit requirements.txt.

Copy link
Collaborator

@combet combet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, all looks good!

@combet combet merged commit 41757b0 into main Nov 19, 2021
@hsinfan1996 hsinfan1996 deleted the issue/437/magnification_bias branch March 17, 2022 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants