Added dV_comoving function and tests. #2103

jbwhit · 2014-02-18T00:57:55Z

Following discussion here:
http://mail.scipy.org/pipermail/astropy/2013-December/002965.html
and @nhmc mentioned it might be useful to have this function included.

The dV_comoving function allows for an easy method of integrating
over a comoving volume. It is useful when, for example,
integrating a function that depends on another function whose
sensitivity changes with redshift.

I've also included tests and a method in the funcs library as well.

astrofrog · 2014-02-18T06:59:45Z

Thanks! I'll let @nhmc and @aconley review this, but I wonder (and @nhmc and @aconley can chime in) whether the function should be called comoving_volume?

jbwhit · 2014-02-18T09:41:43Z

I'm not married to the function name and happy to change it, but, that function name already exists.

The existing comoving_volume function returns the total comoving volume. In contrast, my function, dV_comoving, returns the comoving_volume_element that needs to be integrated over to give the total comoving volume. The reason it's useful is for when one needs to integrate a function over a comoving volume that varies over redshift. This hopefully explains why I used dV instead of volume -- although, again, I have no problem changing the name.

How about comoving_volume_element?

aconley · 2014-02-18T14:55:42Z

Yes, good addition -- I kept thinking somebody should add that. I like comoving_volume_element or possibly differential_comoving_volume.

aconley · 2014-02-18T14:58:42Z

astropy/cosmology/core.py

Maybe we should be doing this, but currently we haven't been giving citations for all formulae inline. The Hogg paper is cited at the top (although not in a docstring...). It might not be crazy to remove the inline citation but convert the comment about the Hogg paper at the top of the file to a sub-package level comment.

embray · 2014-02-18T22:15:09Z

I like differential_ but I have no say in this. Just saw the discussion and found that easiest to process.

Changed relevant doc_strings and tests as well. As discussed at astropy#2103.

jbwhit · 2014-02-19T00:45:11Z

After the discussion here I've:

changed the function name to differential_comoving_volume
removed the reference in the docstring
updated docstring to state that it returns the "Differential comoving volume per redshift per steradian at each input redshift."

I just want to say that the developer documentation for this project is excellent and helped make this process much simpler than it could otherwise have been.

embray · 2014-02-19T16:33:43Z

Hooray for the developer docs! 🎊 Actually I believe there are efforts underway to reorganize them to be even better, so it's good someone thinks they're useful even as is.

aconley · 2014-02-20T23:54:56Z

Looks good to me. Should definitely be added to 0.4.

aconley · 2014-02-21T18:44:20Z

@jbwhit -- sorry, forgot to check this: you should add a line to the CHANGES.rst file under 0.4.0 -- New Features -- astropy.cosmology

nhmc · 2014-02-21T19:01:48Z

astropy/cosmology/core.py

The return value should divided by the steradian unit here, so:

return dh * (zp1 ** 2 * da ** 2) / self.efunc(z) / u.sr

nhmc · 2014-02-21T19:15:23Z

Looks good! I added some inline comments you might want to look at. You should also add the new function in funcs.py to the __all__ list at the top of that file. Once those are done I think it's ready to merge.

jbwhit · 2014-02-24T00:18:24Z

Changes:

Corrected the units: ( / u.sr )
Changed the wording for the description of the function (both core and funcs).
add the new function in funcs.py to the all list at the top of funcs
Added to CHANGES.rst file

jbwhit · 2014-02-24T00:56:12Z

If I'm reading the details of the Travis CI build correctly, it fails for only one setup:

4902.11 6 min 46 sec    16 minutes ago  2.7 SETUP_CMD='test --parallel=8' NUMPY_VERSION=1.8.0 OPTIONAL_DEPS=true LC_CTYPE=C.ascii

I suspect it was adding the u.sr to the return... but I think I'm out of my depth here as to why it's only failing for this one setup, or what to do about it. Any suggestions or comments?

jbwhit · 2014-02-24T02:41:38Z

The only thing different between the passing and failing builds apparently is the:

LC_CTYPE=C.ascii

Flag.

aconley · 2014-02-24T15:58:21Z

That is also (as far as I can tell), the only build that is both parallel and has OPTIONAL_DEPS=true. Not that that seems very useful, but there may be more going on than ASCII. But Travis really does just fail sometimes for no reason.

astrofrog · 2014-02-24T16:22:57Z

It looks like a random failure, so I've restarted it.

In response to comment here: astropy#2103 (comment)

jbwhit · 2014-02-27T01:39:01Z

I'm sorry, but I'm not sure what's breaking during the build or how to fix it. I can revert the code to where it doesn't use the "u.sr" at all, and I suspect it would build. Of course, then the units wouldn't be correct. Any suggestions on what I can be working on?

aconley · 2014-02-27T03:23:31Z

It looks like it's crashing somewhere in time -- I can't see any way this could have anything to do with you. Maybe you branched from a broken version of master or something... I might try rebasing off of the current master, see if that magically fixes it.

After random failures during build, and my failing to rebase properly, I have re-committed the code that was used in this previous pull request: astropy#2103

jbwhit · 2014-04-09T02:49:55Z

Can this be restarted to see if it passes the build? The discussion in #2233 said that this should be fixed here. If it still doesn't build, I'll try to rebase the code and resubmit here.

The dV_comoving function allows for an easy method of integrating over a comoving volume. It is useful when, for example, integrating a function that depends on another function whose sensitivity changes with redshift.

Changed relevant doc_strings and tests as well. As discussed at astropy#2103.

Changed docstring and added the change to the CHANGES.rst file. Also added the function to the __all__ list at the top of funcs.

In response to comment here: astropy#2103 (comment)

The _get_current() is depreciated so changed to the new: _default_cosmology.get()

jbwhit · 2014-05-06T11:27:18Z

Hooray, after rebasing and recommitting I think everything works and is ready to merge!

astrofrog · 2014-05-06T15:21:25Z

Thanks @jbwhit! Merging :)

…ddition Added dV_comoving function and tests.

Changed relevant doc_strings and tests as well. As discussed at astropy#2103.

In response to comment here: astropy#2103 (comment)

Changed relevant doc_strings and tests as well. As discussed at astropy/astropy#2103.

In response to comment here: astropy/astropy#2103 (comment)

astrofrog added the cosmology label Feb 18, 2014

astrofrog added this to the v0.4.0 milestone Feb 18, 2014

aconley reviewed Feb 18, 2014
View reviewed changes

jbwhit added a commit to jbwhit/astropy that referenced this pull request Feb 19, 2014

Renamed function to differential_comoving_volume

fa52c91

Changed relevant doc_strings and tests as well. As discussed at astropy#2103.

nhmc reviewed Feb 21, 2014
View reviewed changes

jbwhit added a commit to jbwhit/astropy that referenced this pull request Feb 25, 2014

Changed the return with units method.

9c99fa1

In response to comment here: astropy#2103 (comment)

jbwhit mentioned this pull request Mar 25, 2014

Recommit pull request due to build failure. #2233

Closed

jbwhit added 6 commits May 6, 2014 17:13

Added dV_comoving function and tests.

2403e91

The dV_comoving function allows for an easy method of integrating over a comoving volume. It is useful when, for example, integrating a function that depends on another function whose sensitivity changes with redshift.

Renamed function to differential_comoving_volume

f1baf71

Changed relevant doc_strings and tests as well. As discussed at astropy#2103.

Changed documentation wording.

797369c

Changed docstring and added the change to the CHANGES.rst file. Also added the function to the __all__ list at the top of funcs.

Changed the return with units method.

2a81acd

In response to comment here: astropy#2103 (comment)

Changed to long form of steradian.

d211ad9

Updated _get_current function call.

7b6bf68

The _get_current() is depreciated so changed to the new: _default_cosmology.get()

astrofrog added the ready-for-final-review label May 6, 2014

astrofrog added a commit that referenced this pull request May 6, 2014

Merge pull request #2103 from jbwhit/feature-comoving-volumeelement-a…

9cd6f89

…ddition Added dV_comoving function and tests.

astrofrog merged commit 9cd6f89 into astropy:master May 6, 2014

astrofrog mentioned this pull request May 6, 2014

Deprecate functional cosmology interface #2343

Merged

embray removed the ready-for-final-review label May 6, 2014

jbwhit deleted the feature-comoving-volumeelement-addition branch May 7, 2014 13:16

ktchrn pushed a commit to ktchrn/astropy that referenced this pull request Oct 28, 2014

Renamed function to differential_comoving_volume

fcbd019

Changed relevant doc_strings and tests as well. As discussed at astropy#2103.

ktchrn pushed a commit to ktchrn/astropy that referenced this pull request Oct 28, 2014

Changed the return with units method.

79c42be

In response to comment here: astropy#2103 (comment)

ntessore pushed a commit to glass-dev/cosmology that referenced this pull request Dec 7, 2020

Renamed function to differential_comoving_volume

ada5f50

Changed relevant doc_strings and tests as well. As discussed at astropy/astropy#2103.

ntessore pushed a commit to glass-dev/cosmology that referenced this pull request Dec 7, 2020

Changed the return with units method.

7626964

In response to comment here: astropy/astropy#2103 (comment)

Uh oh!

Added dV_comoving function and tests. #2103

Added dV_comoving function and tests. #2103

Uh oh!

Conversation

jbwhit commented Feb 18, 2014

Uh oh!

astrofrog commented Feb 18, 2014

Uh oh!

jbwhit commented Feb 18, 2014

Uh oh!

aconley commented Feb 18, 2014

Uh oh!

aconley Feb 18, 2014

Choose a reason for hiding this comment

Uh oh!

embray commented Feb 18, 2014

Uh oh!

jbwhit commented Feb 19, 2014

Uh oh!

embray commented Feb 19, 2014

Uh oh!

aconley commented Feb 20, 2014

Uh oh!

aconley commented Feb 21, 2014

Uh oh!

nhmc Feb 21, 2014

Choose a reason for hiding this comment

Uh oh!

nhmc commented Feb 21, 2014

Uh oh!

jbwhit commented Feb 24, 2014

Uh oh!

jbwhit commented Feb 24, 2014

Uh oh!

jbwhit commented Feb 24, 2014

Uh oh!

aconley commented Feb 24, 2014

Uh oh!

astrofrog commented Feb 24, 2014

Uh oh!

jbwhit commented Feb 27, 2014

Uh oh!

aconley commented Feb 27, 2014

Uh oh!

jbwhit commented Apr 9, 2014

Uh oh!

jbwhit commented May 6, 2014

Uh oh!

astrofrog commented May 6, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants