Angular diameter distance for positive curvature - #4723

I left some inline comments pointing out docstring typos.

Otherwise this looks excellent to me. I much prefer having this generalised comoving distance method (and the new related methods) allowing two input redshifts, instead of the previous approximation used for angular_diameter_distance_z1z2.

@aconley should take a look at this too, though, as he's more of a cosmology guru than me.

Fixed the typos. Thanks!

The previous expression wasn't an approximation. It's analytically exact but numerically unstable.

The functionality looks good to me.

But it does seem a little strange to me that we now have _z1z2 versions of some functions but
not others. And I am a bit leery about adding even more functions to the comology class --
it's getting hard to keep track. Should we instead consider adding default min z arguments
instead of new functions?

For example:
luminosity_distance(self, z1=0, z2)
comoving_transverse_distance(self, z1=0, z2)
absorption_distance(self, z1=0, z2)
angular_diameter_distance(self, z1=0, z2)

we'd have to keep angular_diameter_distance_z1z2 for backwards compatibility,
but it would avoid further fattening of the interface.

I'd be happy to make that change by making a pull against @joergdietrich -s branch
if that seems useful.

Unfortunately, that's a syntax error in Python. Non-default arguments can't follow default arguments.
luminosity_distance(self, z2, z1=0) is counter-intuitive. Maybe luminosity_distance(self, z1, z2=None) is better, which takes computes d_lum(0, z1) if z2 is None and d_lum(z1, z2) if not.

Doing things this way would also allow us to keep the old behavior of raising ValueError for angular_diameter_distance_z1z2 if any z2 < z1 and going ahead with the new, more sensible approach to return negative distances if z2 < z1 in angular_diameter_distance(z1, z2) calls.

Oh, right, nuts.

At that point I think you call them distance(self, zmax, zmin=0).

Will anybody actually use any of the z1z2s except the angular diameter one?

With that in mind, a simpler, fix it now, solution would be to remove (or make private
by adding a leading _) your new z1z2 functions. That solves the immediate problem, but doesn't
force us to commit to supporting the _z1z2 API (which I clearly don't like) in the future
(except for the one that already exists).

@joergdietrich Thanks for clarifying (i.e. it's an exact analytic expression, but numerically unstable).

I don't think the API is a problem here. We can always just replace the existing positional argument with a z1 keyword argument: luminosity_distance(self, z1=None, z2=None)
Then you can pass both as positional or keyword arguments, whatever the user prefers. An error can be raised by hand if z1 is not passed.

But I agree with @aconley's concern about method bloat, and I like his idea to just make the new z1z2 functions private by adding a leading underscore for now.

I like @nhmc's suggestion to make both arguments keyword arguments, although in the end very few methods actually benefit from having both z1 and z2 in practice. Occasionally lookback_time(z1, z2) may be interesting. So I'm okay with making the new methods private for now.

age(z1, z2) would be convenient to have, although easy enough to calculate
without the overloaded version.

But not a suggestion for now.

On Wed, Mar 30, 2016 at 9:58 AM, Jörg Dietrich [email protected]
wrote:

I like @nhmc https://github.com/nhmc's suggestion to make both
arguments keyword arguments, although in the end very few methods actually
benefit from having both z1 and z2 in practice. Occasionally
lookback_time(z1, z2) may be interesting. So I'm okay with making the new
methods private for now.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#4723 (comment)

New commit to make them private for now.

Looks good to me.

@nhmc - are we waiting on something to accept this?

No, it looks ready to merge to me. Sorry for not commenting sooner. Thanks again @joergdietrich.

Yes, thanks for adding this capability. I know lensing folk will find this quite helpful.

One last request before merge -- squash all commits into one. Thanks!

Use the squash button when merging? I know it will take me too much time to figure out how to squash commits already pushed upstream.

Haha... Funny you brought it up. There was a lengthy discussion about that button, resulting in it being disabled. (See #4423 for part of that discussion and astropy-dev mailing list for the rest.)

Well, your choice. I don't have the time for another fight with git.

@aconley , I was going to do it for him but turns out it is quite painful because halfway through the commits, he merged master into his PR. So rebase -i goes wild. I am going to do a brute-force squashing by copying his changes to my own branch and then merge that one. PR coming soon.

I am closing this one now. See #4769 for the squashed version. I eyeballed the changes and made sure I didn't introduce anything new, and added @joergdietrich as the author, so everything should be okay...