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

Ipynb examples #339

Merged
merged 29 commits into from
Oct 27, 2020
Merged

Ipynb examples #339

merged 29 commits into from
Oct 27, 2020

Conversation

itrharrison
Copy link
Contributor

Description

Merging will close #335. Have added iPython notebooks making plots from the outputs of the current example config files.

Checklist

  • Follow the Contributor Guidelines
  • Write unit tests
  • Write documentation strings
  • Assign someone from your working team to review this pull request
  • Assign someone from the infrastructure team to review this pull request

@Lucia-Fonseca
Copy link
Member

Why are tests for power spectrum included here?

@rrjbca rrjbca removed this from the Version 0.3 milestone Oct 9, 2020
Copy link
Member

@Lucia-Fonseca Lucia-Fonseca left a comment

Choose a reason for hiding this comment

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

It looks very good! Great job, Ian.
Two small changes:

  • Add plt.show() to avoid outputing unnecesary text
  • Name of the config file for merger rates
  • I still see test_camb.py and test_class.py.

@jucordero
Copy link
Contributor

jucordero commented Oct 9, 2020

I think you can add a semicolon at the end of a statement
plt.xlabel('This is an x label');
to avoid extra text output in the notebooks without the need of a plt.show

@Lucia-Fonseca
Copy link
Member

That works too, thanks @jucordero!

I think you can add a semicolon at the end of a statement

plt.xlabel('This is an x label');
to avoid extra text output in the notebooks without the need of a plt.show

"\n",
"Running the `merger_rates` example:\n",
"```bash\n",
"$ skypy merger_rates.yml --format fits\n",

This comment was marked as resolved.

Copy link
Contributor

@rrjbca rrjbca left a comment

Choose a reason for hiding this comment

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

Is the intention that the user would run these notebooks themselves? I think they make more sense as part of the documentation (perhaps using nbsphinx?) but if that is the case they should probably be in the docs directory not the examples directory. Particularly because everything in examples is mirrored to the skypyproject/examples repository and we shouldn't distribute them in this way if we don't expect the user to run them.

@Lucia-Fonseca
Copy link
Member

The idea is to have something similar to CAMB DEMO
https://camb.readthedocs.io/en/latest/CAMBdemo.html

Lucia-Fonseca
Lucia-Fonseca previously approved these changes Oct 20, 2020
Copy link
Member

@Lucia-Fonseca Lucia-Fonseca left a comment

Choose a reason for hiding this comment

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

Looks good.

Copy link
Contributor

@rrjbca rrjbca left a comment

Choose a reason for hiding this comment

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

These look good, let's merge them as they are and work out how we want to incorporate them with the documentation in the future.

@rrjbca rrjbca merged commit 1d2b80a into skypyproject:master Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iPython notebooks for existing examples
4 participants