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

Changing fsky to sky_area #314

Merged
merged 6 commits into from
Oct 23, 2020
Merged

Changing fsky to sky_area #314

merged 6 commits into from
Oct 23, 2020
+55 −34

Conversation

philipp128
Copy link
Contributor

@philipp128 philipp128 commented Oct 4, 2020 •
edited
Loading

Description

This PR solves #309 . fsky was changed to sky_area which has to be given in sr or deg2 units.

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

@philipp128 philipp128 added the enhancement Improvement of existing feature label Oct 4, 2020
@philipp128 philipp128 requested a review from a team October 4, 2020 15:40
@philipp128 philipp128 linked an issue Oct 4, 2020 that may be closed by this pull request
change fsky parameters to sky area #309
Closed
@ntessore
Copy link
Member

ntessore commented Oct 5, 2020 •
edited
Loading

Looks good! Could you please use Astropy's quantity_input decorator in the form @quantity_input(sky_area=units.sr) for the functions? Then please remove the explicit conversion you added, and just do

dN_dz = (cosmology.differential_comoving_volume(redshift)*sky_area).to_value('Mpc3')

— Astropy units will do the rest.

@ntessore ntessore added this to the Version 0.4 milestone Oct 5, 2020
@JonathanDHarris
Copy link
Contributor

@ntessore @philipp128 do we need to update the yml files in examples?

@ntessore
Copy link
Member

ntessore commented Oct 5, 2020

@ntessore @philipp128 do we need to update the yml files in examples?

Yes, that's right, good catch!

@philipp128: We don't have an automatic way of constructing unitful quantities yet, so you will need to change fsky: ... to sky_area: !astropy.units.Quantity ['1 deg2'] or so in the examples.

@philipp128
Copy link
Contributor Author

philipp128 commented Oct 5, 2020 •
edited
Loading

Looks good! Could you please use Astropy's quantity_input decorator in the form @quantity_input(sky_area=units.sr) for the functions? Then please remove the explicit conversion you added, and just do

Would this raise an error if the unit is deg2? Cause I think we want to allow this.

Update: Apparently it also accepts deg2 in this case? Does anyone know why?

ntessore
ntessore requested changes Oct 5, 2020
View reviewed changes
Copy link
Member

@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.

some matters of style, but looking good overall

rrjbca
rrjbca requested changes Oct 21, 2020
View reviewed changes
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.

Looks great just a couple of small changes

rrjbca
rrjbca reviewed Oct 22, 2020
View reviewed changes
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.

I just spotted that docs/pipeline/examples/lightcone.yml is still using fsky: 1.0e-6 would recommend changing it to sky_area: 0.05 deg2 so that the plot still has a similar number of galaxies to before.

@rrjbca rrjbca requested a review from ntessore October 23, 2020 11:38
ntessore
ntessore approved these changes Oct 23, 2020
View reviewed changes
Copy link
Member

@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.

LGTM

@rrjbca rrjbca merged commit 4a852cb into skypyproject:master Oct 23, 2020
@philipp128 philipp128 deleted the sky_fraction branch November 26, 2020 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ntessore ntessore ntessore approved these changes

@rrjbca rrjbca rrjbca approved these changes

Assignees
No one assigned
Labels
enhancement Improvement of existing feature module: galaxies
Projects
None yet
Milestone
Version 0.4
Development

Successfully merging this pull request may close these issues.

change fsky parameters to sky area
4 participants
@philipp128 @ntessore @JonathanDHarris @rrjbca