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/438/cluster wl simple #461

Open
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

eduardojsbarroso
Copy link
Contributor

@eduardojsbarroso eduardojsbarroso commented Oct 11, 2024

Description

Implementation of cluster shear profile analysis in Firecrown.

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist:

The following checklist will make sure that you are following the code style and
guidelines of the project as described in the
contributing page.

  • I have run bash pre-commit-check and fixed any issues
  • I have added tests that prove my fix is effective or that my feature works
  • I have made corresponding changes to the documentation
  • I have 100% test coverage for my changes (please check this after the CI system has verified the coverage)

@@ -107,11 +107,11 @@ def _compute_theory_vector(self, tools: ModelingTools) -> TheoryVector:
if cl_property == ClusterProperty.COUNTS:
theory_vector_list += cluster_counts
continue

if cl_property == ClusterProperty.DELTASIGMA:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before, this was giving an error if the property was neither COUNTS or MEAN_MASS. However, the way the code is structure now, I want to be able to pass two statistics for the likelihood, and this statistics will not do anything with the DELTASIGMA data type. It has not to give an error because the other statistics will read this data (I have one SACC file with all the three data types). Nonetheless, should we test it here if all the properties provided are at least in the ClusterProperty module? Is there a good way to do it besides adding a for loop here? @m-aguena @vitenti

@@ -63,7 +63,7 @@ def get_observed_data_and_indices_by_survey(
# pylint: disable=no-member
data_type = sacc.standard_types.cluster_mean_log_mass
else:
raise NotImplementedError(cluster_property)
continue
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is another example from my last comment. I want it to go through the data and just do not do anything if it is not a property of the statiscs. However this should be replaced with another error if the provided properties are not the ones from the ClusterProperty module

@eduardojsbarroso
Copy link
Contributor Author

@vitenti All the tests pass in my local computer. The problem is that the new modules need clmm and the miniforge does not have it. Are we going to add clmm as a requirement for Firecrown?

@marcpaterno
Copy link
Collaborator

marcpaterno commented Oct 17, 2024

Since clmm is a DESC product, I think we should request that it be made conda-installable.

@eduardojsbarroso
Copy link
Contributor Author

Since clmm is a DESC product, I think we should request that it be made conda-installable.

I think there was some discussion about it, but I don't know if this will be done soon.

@vitenti
Copy link
Collaborator

vitenti commented Oct 18, 2024

Since clmm is a DESC product, I think we should request that it be made conda-installable.

I think there was some discussion about it, but I don't know if this will be done soon.

@eduardojsbarroso
Preparing a conda-forge package is not technically challenging; the main difficulty lies in identifying all the required dependencies and checking if they are already available on conda-forge. Additionally, if the package includes third-party code, it's necessary to ensure the licenses are compatible. If you're interested in creating such a package, I'd be happy to assist you.

@eduardojsbarroso
Copy link
Contributor Author

Since clmm is a DESC product, I think we should request that it be made conda-installable.

I think there was some discussion about it, but I don't know if this will be done soon.

@eduardojsbarroso Preparing a conda-forge package is not technically challenging; the main difficulty lies in identifying all the required dependencies and checking if they are already available on conda-forge. Additionally, if the package includes third-party code, it's necessary to ensure the licenses are compatible. If you're interested in creating such a package, I'd be happy to assist you.

I will take a look at this then and message you once I found some problems. Thank you @vitenti

@eduardojsbarroso
Copy link
Contributor Author

Since clmm is a DESC product, I think we should request that it be made conda-installable.

I think there was some discussion about it, but I don't know if this will be done soon.

@eduardojsbarroso Preparing a conda-forge package is not technically challenging; the main difficulty lies in identifying all the required dependencies and checking if they are already available on conda-forge. Additionally, if the package includes third-party code, it's necessary to ensure the licenses are compatible. If you're interested in creating such a package, I'd be happy to assist you.

I will take a look at this then and message you once I found some problems. Thank you @vitenti

Hi @vitenti, is it possible to add a package on the Firecrown dependencies from direct links to a github? Because I know in TXPipe they use:

pip git+https://github.com/LSSTDESC/CLMM.git@1.14.1 

Can we add this line to the environment.yml file? Otherwise I am trying to publish CLMM to conda but there is one direct link like this one that does not allow me to do it, so I will try to contour the problem.

@vitenti
Copy link
Collaborator

vitenti commented Nov 12, 2024

The qp package is already on conda-forge https://github.com/conda-forge/qp-feedstock . Does CLMM have a reason to use a direct pip install?

@eduardojsbarroso
Copy link
Contributor Author

The qp package is already on conda-forge https://github.com/conda-forge/qp-feedstock . Does CLMM have a reason to use a direct pip install?

I think that the installation was done before the qp package was published to Pypi. I managed to publish CLMM to the PypiTest (here). Now I am waiting for someone on the CLMM side to give me the aproval to publish to conda and Pypi

@eduardojsbarroso
Copy link
Contributor Author

Hi @vitenti , clmm is now on the conda-forge repository

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.

5 participants