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/401/use qp+fix zinteg #644

Merged
merged 53 commits into from
Oct 7, 2024
Merged

Issue/401/use qp+fix zinteg #644

merged 53 commits into from
Oct 7, 2024

Conversation

m-aguena
Copy link
Collaborator

@m-aguena m-aguena commented Sep 26, 2024

Using qp for handling PDFs. The main changes are:

  • The interpolation of the PDFs on a finer grid for integration in _integ_pzfuncs is done by qp.
  • Adding the possibility of having quantiles as the PDF information:
    • Added this option in generate_galaxy_catalog
    • Makes the GCData object convert the quantiles into a PDF grid with GCData.get_pzpdfs, which is used in the integration mentioned above.

Closes #401 and #643

@m-aguena m-aguena linked an issue Sep 26, 2024 that may be closed by this pull request
@coveralls
Copy link

coveralls commented Sep 26, 2024

Coverage Status

coverage: 100.0%. remained the same
when pulling be75a04 on issue/401/use_qp+fix_zinteg
into 7d90ee1 on main.

Copy link
Collaborator

@hsinfan1996 hsinfan1996 left a comment

Choose a reason for hiding this comment

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

  1. Should we add qp to requirements.txt and the Requirements section in README?

  2. It seems to me that the imports in some of the files are not compliant with PEP8, but since pylint doesn't complain about C0411 / wrong-import-order, it is not a big deal. I am just wondering.

clmm/gcdata.py Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

This new notebook is very useful to see the various behaviors, but the markdown text needs to be corrected/expanded to help the user understand better what is happening. At the moment, the markdown seems to copied/pasted from another demo notebook and gives little information as to what is actually happening.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be good to add a legend in the figure comparing the pdz computed for the 3 types (individual, shared and quantiles)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, maybe add some explanation text for the last figure of the notebook. I find trick to understand what is actually shown.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And why is the qp integral performing worse than the clmm-implemented ones but for the "quantiles" case where qp appears to perform better?

Copy link
Collaborator Author

@m-aguena m-aguena Sep 30, 2024

Choose a reason for hiding this comment

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

Thanks for the comments! I added more explanation to the notebook

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 think that qp is performing worse than clmm because it kind of used a type like integral with the precision set by the separation of the zbins of the input provided, and in clmm we are refining the grid for the integral.

In the case of using quantiles, I think qp might be outperforming clmm because it is actually using the statistical meaning of quantiles while clmm recreates an interpolated PDF and integrates it, thus loosing information.

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 @m-aguena. It's great to see qp and the quantile option available!

I've tested the code and everything appears to be working well. I just have comments about the new notebook showing the integral with qp versus clmm, etc. At the moment, I don't think they are enough or clear enough markdown explanations to easily understand what this is doing.

(And sorry, I clicked on "comment-only" for some comments and on "review" so they don't all appear in the "review" below)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, maybe add some explanation text for the last figure of the notebook. I find trick to understand what is actually shown.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And why is the qp integral performing worse than the clmm-implemented ones but for the "quantiles" case where qp appears to perform better?

@hsinfan1996
Copy link
Collaborator

hsinfan1996 commented Sep 30, 2024

I added qp (qp-prob) to both requirements.txt and environment.yml.

INSTALL.md Outdated Show resolved Hide resolved
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.

Thanks for adding more explanations to the notebook. All looks good to me know!

@m-aguena m-aguena merged commit 157bdb0 into main Oct 7, 2024
2 checks passed
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.

Computation of PDF integral Add p(z) to GalaxyCluster object
4 participants