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.

Sign up for GitHub

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

Jump to bottom

NLA model for intrinsic alignments #21

Merged
merged 7 commits into from
Oct 15, 2024
Merged

NLA model for intrinsic alignments #21

merged 7 commits into from
Oct 15, 2024

Conversation

mwiet
Copy link
Contributor

@mwiet mwiet commented Jul 27, 2022 •
edited by github-actions bot
Loading

The ia_nla function computes the effective convergence from intrinsic alignments, calculated using the Non-linear Aligments Model (NLA). The convergence from intrinsic aligments is follows Catelan et al. (2000), Hirata & Seljak (2004), Bridle & King (2007).

Added: The new kappa_ia_nla() function computes an effective convergence due to Intrinsic Alignments using the NLA model.
Reviewed-by: Nicolas Tessore
Reviewed-by: Arthur Loureiro [email protected]

@arthurmloureiro
Copy link
Contributor

Hi @mwiet,

Can you add documentation with references and equations to the docstring in the NLA model?

There are templates for this in the other functions :)

@mwiet
Copy link
Contributor Author

mwiet commented Jul 27, 2022

Hi there! Thanks for letting me know! The documentation and references are now included in the function's docstring.

ntessore
ntessore requested changes Jul 28, 2022
View reviewed changes
Copy link
Collaborator

@ntessore ntessore left a comment

Choose a reason for hiding this comment

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

Great first PR! Only a couple of minor changes.

Could you also add your new generator to the documentation by including it in the list at the top of the file?

glass/lensing.py Outdated Show resolved Hide resolved
glass/lensing.py Outdated Show resolved Hide resolved
glass/lensing.py Outdated Show resolved Hide resolved
glass/lensing.py Outdated Show resolved Hide resolved
glass/lensing.py Outdated Show resolved Hide resolved
glass/lensing.py Outdated Show resolved Hide resolved
ntessore
ntessore requested changes Jul 28, 2022
View reviewed changes
Copy link
Collaborator

@ntessore ntessore left a comment

Choose a reason for hiding this comment

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

That's great! Please take a look at the generated documentation (the link is under the "checks" in this PR), there are a couple of issues there (e.g. the references aren't linked, and math errors).

glass/lensing.py Outdated Show resolved Hide resolved
glass/lensing.py Outdated Show resolved Hide resolved
@ntessore ntessore self-assigned this Sep 30, 2022
@ntessore ntessore added enhancement New feature or request ⚡ galaxies and removed ⚡ lensing labels Sep 30, 2022
@ntessore ntessore added this to the paper release milestone Sep 30, 2022
@ntessore
Copy link
Collaborator

Closing this, as the code is copied from the bornraytrace package without attribution. We will create an extension using that package instead.

@ntessore ntessore closed this Feb 28, 2023
@ntessore ntessore deleted the ia_nla branch February 28, 2023 18:26
@ntessore ntessore restored the ia_nla branch March 21, 2023 14:23
@ntessore ntessore reopened this Mar 21, 2023
@ntessore
Copy link
Collaborator

Ok, the issue has been resolved! I think we can try and merge this again. I will let @NiallJeffrey review this PR.

@ntessore ntessore changed the title Inclusion of the NLA model for intrinsic alignments ENH: NLA model for intrinsic alignments Mar 21, 2023
@ntessore ntessore modified the milestones: v2023.2, v2023.3 Mar 21, 2023
@ntessore ntessore modified the milestones: v2023.3, v2022.4 Apr 3, 2023
@ntessore ntessore changed the title ENH: NLA model for intrinsic alignments ENH(galaxies): NLA model for intrinsic alignments May 18, 2023
@mwiet
Copy link
Contributor Author

mwiet commented May 22, 2023 via email

@ntessore ntessore modified the milestones: v2023.5, v2023.6 May 31, 2023
@ntessore ntessore removed this from the v2023.6 milestone Jun 30, 2023
ntessore
ntessore approved these changes Mar 19, 2024
View reviewed changes
Copy link
Collaborator

@ntessore ntessore left a comment

Choose a reason for hiding this comment

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

Code looks good to me. Pinging @NiallJeffrey for scientific review.

@ntessore ntessore added the science Science improvement or question label Mar 19, 2024
arthurmloureiro
arthurmloureiro reviewed May 14, 2024
View reviewed changes
Copy link
Contributor

@arthurmloureiro arthurmloureiro left a comment

Choose a reason for hiding this comment

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

Let's merge this asap :D

glass/galaxies.py Show resolved Hide resolved
glass/galaxies.py Outdated Show resolved Hide resolved
@arthurmloureiro
Copy link
Contributor

@ntessore I think we can merge this :)

@paddyroddy
Copy link
Member

Have just updated from current main

@paddyroddy
Copy link
Member

pre-commit.ci autofix

@paddyroddy
Copy link
Member

Have addressed linting issues so that the CI passes. When the science is ready, this PR is ready to go 🙂

@arthurmloureiro
Copy link
Contributor

From the science pov, both me and @mwiet have reviewed it and it seems solid.

But I think it would be great to have @NiallJeffrey to take a quick look :)

@paddyroddy paddyroddy mentioned this pull request Oct 10, 2024
Merged
@paddyroddy
Copy link
Member

Have done another update on this PR, just so it follows the practices in main - namely docstrings, mypy support etc. Looks like this in the docs
image

@ntessore
Copy link
Collaborator

I think this can finally go in.

@NiallJeffrey speak now or forever hold your peace 😉

@NiallJeffrey
Copy link

This looks good to me!

Comment -- it might be useful to put the C1 value in the docs, but it is the industry-standard value, so not essential

@ntessore ntessore changed the title ENH(galaxies): NLA model for intrinsic alignments NLA model for intrinsic alignments Oct 15, 2024
NiallJeffrey
NiallJeffrey approved these changes Oct 15, 2024
View reviewed changes
Copy link

@NiallJeffrey NiallJeffrey left a comment

Choose a reason for hiding this comment

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

The implementation appears corrects to me

@paddyroddy
Copy link
Member

I'm going to merge this before it falls stale again

@paddyroddy paddyroddy merged commit 60dc668 into main Oct 15, 2024
8 of 10 checks passed
@paddyroddy paddyroddy deleted the ia_nla branch October 15, 2024 16:25
paddyroddy
paddyroddy reviewed Nov 8, 2024
View reviewed changes
glass/galaxies.py
rho_c1 = c1 * cosmo.rho_c0

prefactor = -a_ia * rho_c1 * cosmo.Om
inverse_linear_growth = 1.0 / cosmo.gf(zeff)
Copy link
Member

Choose a reason for hiding this comment

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

What is this gf? This function should really have been added with tests. I'm not sure if it actually works...

cc: @ntessore (found in #397)

Copy link
Member

Choose a reason for hiding this comment

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

Related search https://github.com/search?q=repo%3Aglass-dev%2Fcosmology+gf&type=code

paddyroddy added a commit that referenced this pull request Nov 8, 2024
@paddyroddy
Revert "NLA model for intrinsic alignments (#21)"
0a83b2e 
This reverts commit 60dc668.
This was referenced Nov 8, 2024
Draft
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paddyroddy paddyroddy paddyroddy left review comments

@NiallJeffrey NiallJeffrey NiallJeffrey approved these changes

@arthurmloureiro arthurmloureiro arthurmloureiro approved these changes

@ntessore ntessore ntessore approved these changes

Assignees

@ntessore ntessore

Labels
enhancement New feature or request science Science improvement or question
Projects
None yet
Milestone
v2024.2
Development

Successfully merging this pull request may close these issues.
5 participants
@mwiet @arthurmloureiro @ntessore @paddyroddy @NiallJeffrey