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

ENH: Logistic completeness function #521

Merged
merged 12 commits into from
May 25, 2022

Conversation

philipp128
Copy link
Contributor

@philipp128 philipp128 commented Feb 22, 2022

Description

This PR adds a feature to calculate the logistic completeness function. Closes #519.

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 enhancement Improvement of existing feature module: utils labels Feb 22, 2022
@philipp128 philipp128 requested a review from a team as a code owner February 22, 2022 07:24
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.

Comments on readability, naming convention and potentially extra unit tests.

skypy/utils/completeness_function.py Outdated Show resolved Hide resolved
skypy/utils/completeness_function.py Outdated Show resolved Hide resolved
skypy/utils/completeness_function.py Outdated Show resolved Hide resolved
def logistic_completeness_function(m, m95, m50):
r'''Compute logistic completeness function.

This function calculates the logistic completeness function
Copy link
Member

Choose a reason for hiding this comment

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

Can you provide a reference?

skypy/utils/completeness_function.py Outdated Show resolved Hide resolved
skypy/utils/tests/test_completeness_function.py Outdated Show resolved Hide resolved
skypy/utils/tests/test_completeness_function.py Outdated Show resolved Hide resolved
skypy/utils/tests/test_completeness_function.py Outdated Show resolved Hide resolved
skypy/utils/tests/test_completeness_function.py Outdated Show resolved Hide resolved
skypy/utils/tests/test_completeness_function.py Outdated Show resolved Hide resolved
@philipp128
Copy link
Contributor Author

philipp128 commented Feb 25, 2022

Edit: It works now and the docs should be fine.

Docs are failing. I am waiting to resolve this right now cause I have a problem with the docs anyway.

The docs for completeness function is not rendered properly. It is included but not linked or specified -> see attached image. I do not know where the problem is.

@rrjbca do you know whether I forgot sth?

image

@rrjbca
Copy link
Contributor

rrjbca commented Mar 11, 2022

I would also suggest making this part of skypy.utils.photometry. In effect the completeness is an extrinsic photometric property of the sources. This would again save creating a new submodule.

Lucia-Fonseca
Lucia-Fonseca previously approved these changes May 9, 2022
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.

Just a comment on docstrings, but looks good.

skypy/utils/photometry.py Outdated Show resolved Hide 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.

  • You shouldn't need six tests for different combinations of array shapes; one slightly more complex broadcasting test should be sufficient e.g:
m = np.full((2, 1, 1), 21)
m95 = np.full((3, 1), 22)
m50 = np.full(5, 23)
p = logistic_completeness_function(m, m95, m50)
assert p.shape = np.broadcast(m, m95, m50).shape
  • In your final test check more magnitudes that have analytic values:
m95 = 24
m50 = 25
m = [np.finfo(np.float64).min, m95, m50, 2*m50-m95, np.finfo(np.float64).max]
p = logistic_completeness_function(m, m95, m50)
assert np.allclose(p, [1, 0.95, 0.5, 0.05, 0])
  • The reference to López-Sanjuan 2017 is not linked anywhere in the docstring. Also note the correct spelling of the name with the accent.

@rrjbca rrjbca merged commit 22ebe38 into skypyproject:main May 25, 2022
itrharrison added a commit that referenced this pull request Dec 15, 2022
* Update name of default branch to main (#434)

* update mailmap (#432)

* Write all tables to a single FITS/HDF5 file (#425)

* ADR 3: Position sampling and patches of the sky (#422)

* BUG: Raise ImportError if optional dependency speclite is not installed (#437)

* MAINT: Set NumPy latest supported version to 1.20 #440

* Update status badges (#441)

* MAINT: Update Lucia affiliation (#451)

* MAINT: add SIT's information (#450)

* DOC: Fix contributor guidelines link (#449)

Co-authored-by: Richard R <58728519+rrjbca@users.noreply.github.com>
Co-authored-by: Nicolas Tessore <n.tessore@ucl.ac.uk>

* ENH: Logging for skypy command line script and Pipeline class (#453)

* DOC: Describe speclite filters in documentation (#457)

* ENH: Config syntax for importing objects (#463)

* DOC: List of Features (#456)

* DOC: How to construct config files (#454)

* DOC: Remove docstring examples (#429)

* MAINT: Update Zenodo entry for RPR (#468)

* DOC: Readme updates (#460)

* DOC: Expanded landing page documentation (#228)

* DOC: Inverse transform sampling accuracy warning (#472)

* MAINT: Set astropy latest supported version to 4.2 (#483)

* DOC: zenodo json members update (#481)

* DOC: Ryden04 ellipticity doc missing section (#477)

* MAINT: Update numpy and scipy latest supported versions (#488)

* BUG: Change invalid ecsv datatype from int to uint16 (#485)

* DEV: setuptools==58.0.0 (#493)

Co-authored-by: Nicolas Tessore <n.tessore@ucl.ac.uk>

* Add compatibility workflow badge (#487)

* DEV: Enable pip to install pre-releases in the tox dev environments (#491)

* TST: Use tmp_path fixture for temporary files in unit tests (#489)

* BUG: Move handling of context arguments after handling of .depends keyword (#465)

* BLD: Set astropy latest supported version to 4.3 and speclite minversion to 0.14 (#486)

* REV: restore setuptools to latest version on readthedocs (#494)

* DEV: pyparsing<3.0.0 (#500)

* Check new astropy file overwrite error message in logging test (#498)

* REV: restore pyparsing to latest version for doc builds (#501)

* DOC: Update citation file with JOSS paper reference (#496)

* BLD: Set astropy latest supported version to 5.0 (#504)

* BLD: Set python latest supported version to 3.10 (#505)

* BLD: Set numpy latest supported version to 1.22 (#506)

* BLD: Set python oldest supported version to 3.7 (#507)

* DOC: Fix code of conduct link (#508)

* Changed y-label in luminosity function example. (#512)

* BLD: Set scipy latest supported version to 1.8 (#510)

* ENH: Rykoff model of the magnitude uncertainty (#526)

* TST: assert photometric error is numerically close to the analytic value (#545)

* TST: Drop deprecated astropy.tests.helper.raises (#546)

* ENH: compute kcorrect remaining stellar mass (#476)

* compute kcorrect remaining stellar mass

* added test for stellar mass remain

Co-authored-by: Ian Harrison <itrharrison@gmail.com>
Co-authored-by: Lucia F. de la Bella <55983939+Lucia-Fonseca@users.noreply.github.com>

* ENH: Logistic completeness function (#521)

* BLD: Set astropy latest supported version to 5.1 (#547)

* BUG: `schechter_smf` callable input and docs (#525)

* DOC: Typo in Rykoff error (#550)

* add Fox's details (#551)

Co-authored-by: Richard R <58728519+rrjbca@users.noreply.github.com>

* BLD: Set numpy latest supported version to 1.23 (#552)

* codestyle fixes

* add six requirement for colossus

* tried to fix docs builds

* update passenv

* rtd configuration

Co-authored-by: Richard R <58728519+rrjbca@users.noreply.github.com>
Co-authored-by: Nicolas Tessore <n.tessore@ucl.ac.uk>
Co-authored-by: Lucia F. de la Bella <55983939+Lucia-Fonseca@users.noreply.github.com>
Co-authored-by: Sut-Ieng Tam <30295725+sutieng@users.noreply.github.com>
Co-authored-by: philipp128 <48715661+philipp128@users.noreply.github.com>
Co-authored-by: Fox Davidson <93545862+Fox-Davidson@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement of existing feature module: utils
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: Logistic Completeness function
3 participants