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

Update install readme #45

Merged
merged 17 commits into from
Jul 2, 2024
Merged

Update install readme #45

merged 17 commits into from
Jul 2, 2024

Conversation

arthurmloureiro
Copy link
Collaborator

@arthurmloureiro arthurmloureiro commented Jul 1, 2024

Updates the installation instructions in the readme file to create/update environments with the dependencies.

Following the current instructions resulted in missing dependencies such as TJPCov.

It also fixes the cosmosis examples that needed updating.

@arthurmloureiro arthurmloureiro added the documentation Improvements or additions to documentation label Jul 1, 2024
@arthurmloureiro arthurmloureiro self-assigned this Jul 1, 2024
README.md Outdated
conda activate my_env
conda env update --name my_env --file=environment.yml --prune
```
and activate your environment with `conda activate [my_env/forecasting]`.

Next install firecrown and augur.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The conda environment created by the previous conda env create command already includes Firecrown. Unless the Augur developers are planning to also modify Firecrown code, there is no reason to clone the Firecrown repository and then to pip-install Firecrown into the environment a second time.

For those Augur developers who do want to modify Firecrown code, it would be much better to have another YAML file to create an environment that does not contain Firecrown (but that does contain all of the things upon which Firecrown depends). That is complicated; in Firecrown we provide such an environment file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@marcpaterno if this is about lines 34-35: this was supposed to be an alternative for updating an env the user has, instead of creating a new one.

The Next install firecrown was there before, but I agree that it should be modified to reflect that this step is only needed if the user wants to modify firecrown. I will update the readme to reflect this, if you agree!

Copy link
Collaborator

Choose a reason for hiding this comment

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

@arthurmloureiro I agree with removing the instructions to clone Firecrown and then pip-install it. But I was also pointing out that if someone working with Augur does want to modify Firecrown code, they should not also have Firecrown (with an older version) already installed in the conda environment into which the new pip-install will be done. It can be quite tricky to make sure that such an environment has consistent versions of everything to work together correctly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@marcpaterno I pointed to the developer installation instructions from Firecrown :)

@arthurmloureiro arthurmloureiro marked this pull request as draft July 1, 2024 14:17
@arthurmloureiro
Copy link
Collaborator Author

CI is breaking because of this:
healpy/healpy#952

Scipy changed the namespace from trapz to trapezoid and this breaks Healpy as heapy has not been updated in PYPI yet. I tried imposing an older version of scipy to avoid this but github CI is ignoring this for some unknown reason.

@marcpaterno
Copy link
Collaborator

CI is breaking because of this: healpy/healpy#952

Scipy changed the namespace from trapz to trapezoid and this breaks Healpy as heapy has not been updated in PYPI yet. I tried imposing an older version of scipy to avoid this but github CI is ignoring this for some unknown reason.

In Firecrown we encountered failures in a library being called from CCL that was not yet consistent with the SciPy 1.14 release. We handled the problem by putting a requirement of scipy < 1.14 into Firecrown's environment.yml. The same may work for Augur.

@arthurmloureiro arthurmloureiro marked this pull request as ready for review July 1, 2024 15:23
@arthurmloureiro
Copy link
Collaborator Author

This PR also fixes issues with the cosmosis example.

Copy link
Collaborator

@fjaviersanchez fjaviersanchez 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 so much!! It looks great to me.

@arthurmloureiro arthurmloureiro merged commit 73851cf into master Jul 2, 2024
1 check passed
@arthurmloureiro arthurmloureiro deleted the update_install_readme branch July 2, 2024 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants