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

Remove code deprecated in 0.3 and 0.4 #2990

Merged
merged 6 commits into from Sep 30, 2014

Conversation

astrofrog
Copy link
Member

My understanding of the deprecation process is that we are deprecating functions/classes/methods in one stable version and removing it in the next. This PR removes code marked as deprecated in 0.3 and 0.4 since it should be removed before 1.0. I was thinking it would make sense to do this sooner rather than later so that packages that test against 1.0 won't see these functions disappear at the last minute before the 1.0 release. We shouldn't backport this to v0.4.x though.

@embray - there is actually a fair bit of deprecated code in astropy.io.fits - shall we remove it soon so that it doesn't get included in 1.0?

cc @eteq @embray @mdboom @taldcroft @nhmc @aconley (since this includes changes in one of your subpackages)

@astrofrog astrofrog added this to the v1.0.0 milestone Sep 28, 2014
@astrofrog
Copy link
Member Author

@nhmc @aconley - luminosity_distance was deprecated in 0.4, but Distance.compute_z still uses it, hence the failure. How should we deal with this?

@astrofrog
Copy link
Member Author

By the way, we can also decide to not merge this, I don't feel strongly about it, but we should remove code that we agree should be removed to avoid clutter.

@taldcroft
Copy link
Member

👍 for removing the deprecated items.

@astrofrog
Copy link
Member Author

@nhmc @aconley @eteq - the failure is due to an issue in astropy.coordinates in the Distance class - see #2991

@nhmc
Copy link
Contributor

nhmc commented Sep 29, 2014

This looks great to me, once #2991 is merged.

@aconley
Copy link
Contributor

aconley commented Sep 29, 2014

Looks like I missed the entire discussion and resolution, but, yes, looks good to me, assuming #2991 fixes the Travis fail.

@mdboom
Copy link
Contributor

mdboom commented Sep 29, 2014

👍

@mhvk
Copy link
Contributor

mhvk commented Sep 29, 2014

Great!

@embray
Copy link
Member

embray commented Sep 29, 2014

👍

@astrofrog
Copy link
Member Author

@embray - shall I also remove deprecated code from io.fits or will you see to that separately?

@embray
Copy link
Member

embray commented Sep 29, 2014

I'll see to that separately (I have already done that in PyFITS so it's just a matter of merging over).

@eteq
Copy link
Member

eteq commented Sep 29, 2014

👍 once #2991 is merged and makes the tests pass.

@astrofrog
Copy link
Member Author

@nhmc @aconley - I just removed some of the tests for astropy.cosmology that I think are no longer relevant - could you double check that I did not remove too much?

@astrofrog
Copy link
Member Author

@nhmc @aconley - I found I had to update test_z_at_value_roundtrip so can you review that carefully? In particular, I had to mark all the methods as being skipped:

    skip = ('Ok', 'angular_diameter_distance_z1z2', 'clone', 'de_density_scale', 'w')

Shouldn't Ok, de_density_scale, and w work though? (they fail currently, hence why I skip them).

# we need zmax here to pick the right solution for
# angular_diameter_distance and related methods.
assert np.allclose(z, funcs.z_at_value(f, fval, zmax=1.5))
Copy link
Member Author

Choose a reason for hiding this comment

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

@nhmc @aconley - the above function is what I was referring to - I'm not sure if I should be skipping Ok, de_density_scale and w - maybe they should just work? (in which case it indicates a bug)

Copy link
Contributor

Choose a reason for hiding this comment

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

For the Planck 13 cosmology (flat, w=-1), Ok, de_density_scale, and w are redshift independent. So z_at_value can't invert the function. If you want to test those three, you will need to create a more 'interesting' cosmology.

@aconley
Copy link
Contributor

aconley commented Sep 30, 2014

@astrofrog -- other than the round trip test, looks good. And for the round trip test, I don't think there's a problem, it's just that you have to decide if you want to explicitly test those functions (which will require a different test cosmology).

@astrofrog
Copy link
Member Author

@aconley - ok, thanks for the clarification!

Since there were no objections above, I'll go ahead and merge :)

astrofrog added a commit that referenced this pull request Sep 30, 2014
Remove code deprecated in 0.3 and 0.4
@astrofrog astrofrog merged commit 67a50ac into astropy:master Sep 30, 2014
@embray
Copy link
Member

embray commented Sep 30, 2014

Changes that remove more code than they add are always the best.

@astrofrog astrofrog deleted the remove-deprecated branch July 5, 2016 18:46
ntessore pushed a commit to glass-dev/cosmology that referenced this pull request Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants