Skip to content

Conversation

sarahbridle
Copy link
Contributor

@sarahbridle sarahbridle commented Sep 22, 2020

Description

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

@rrjbca rrjbca changed the title Placeholder for halo circular velocity function Halo circular velocity function from Maller & Bullock 2004 Sep 24, 2020
@rrjbca rrjbca requested review from a team September 24, 2020 10:09
@rrjbca rrjbca added module: halos new feature New feature, such as a new model labels Sep 24, 2020
@rrjbca rrjbca linked an issue Sep 24, 2020 that may be closed by this pull request
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.

Great start! A few suggestions to improve the code:

  • We prefer to avoid abbreviations in variable names where practical, so M should probably be renamed mass
  • We try to use astropy units where appropriate. So you should do from astropy.units import Unit and then e.g. 96.6 * Unit('km s-1') and (mass.to_value('Msun') / 1e11)
  • Use cosmology.h rather than cosmology.H0 / 100. This also has the correct units.
  • We need a test for the function. See e.g. test_carroll for a simple example of writing tests. Create a new file skypy/halo/tests/test_properties.py and write a function test_circular_velocity. An easy test would be to choose the arguments such that each bracket in the function equals one and check that the returned value equals 96.6 km/s. Perhaps you can think of others
  • There are some trivial formatting issues relating to whitespace/tabs/blank lines etc. At the bottom of the pull request next to "codeclimate" click "details" to see the suggestions

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 needs some changes and tests.
Please, take this review as suggestions.

@Lucia-Fonseca
Copy link
Member

We should update the description to link with the correspondent issue.

@rrjbca rrjbca changed the base branch from master to main February 22, 2021 13:39
@itrharrison itrharrison mentioned this pull request Mar 24, 2022
5 tasks
@itrharrison
Copy link
Contributor

Replaced with #529 to change target branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: halos new feature New feature, such as a new model
Projects
None yet
Development

Successfully merging this pull request may close these issues.

halo circular velocity
5 participants