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 deprecated use of class composition in modeling docs #8736

Merged
merged 2 commits into from May 20, 2019

Conversation

Projects
None yet
4 participants
@astrofrog
Copy link
Member

commented May 20, 2019

There were a couple of cases in the docs where we still used composition with classes, and this also fixes a whitespace issue in the deprecation warning.

I'm tagging this as v3.2 since we shouldn't be showing any deprecated functionality in the docs.

cc @nden @perrygreenfield

@astrofrog astrofrog added this to the v3.2 milestone May 20, 2019

@astrofrog astrofrog requested a review from nden May 20, 2019


plt.figure(figsize=(8, 5))
plt.plot(x, g0(x), 'g--', label='$z=0$')

for z in (0.2, 0.4, 0.6):
g = RedshiftedGaussian(z_0=z)
g = RedshiftScaleFactor(z) | Gaussian1D(1, 0.75, 0.1)

This comment has been minimized.

Copy link
@astrofrog

astrofrog May 20, 2019

Author Member

Is there a better way to write this example?

@astrofrog astrofrog requested a review from perrygreenfield May 20, 2019

@astrofrog astrofrog force-pushed the astrofrog:remove-compound-class branch from 7500887 to 86364a4 May 20, 2019

@bsipocz

This comment has been minimized.

Copy link
Member

commented May 20, 2019

This reminds me that we should probably run the tests once locally to check where we run into deprecationwarnings inside astropy proper.

@codecov

This comment has been minimized.

Copy link

commented May 20, 2019

Codecov Report

Merging #8736 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8736   +/-   ##
=======================================
  Coverage   86.95%   86.95%           
=======================================
  Files         399      399           
  Lines       59351    59351           
  Branches     1100     1100           
=======================================
  Hits        51609    51609           
  Misses       7101     7101           
  Partials      641      641
Impacted Files Coverage Δ
astropy/modeling/core.py 90.21% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa90993...86364a4. Read the comment docs.

@bsipocz

This comment has been minimized.

Copy link
Member

commented May 20, 2019

@astrofrog - would you mind updating the relevant tests below to catch the deprecation warnings for the compound models?


============================================================================= ERRORS ==============================================================================
____________________________________________________ ERROR collecting astropy/modeling/tests/test_compound.py _____________________________________________________
astropy/modeling/tests/test_compound.py:737: in <module>
    (_ConstraintsTestA + _ConstraintsTestB)()])
astropy/modeling/core.py:77: in _opfunc
    AstropyDeprecationWarning)
E   astropy.utils.exceptions.AstropyDeprecationWarning: Composition of model classes will be removed in 4.0(but composition of model instances is not affected)
____________________________________________________ ERROR collecting astropy/modeling/tests/test_compound.py _____________________________________________________
astropy/modeling/tests/test_compound.py:737: in <module>
    (_ConstraintsTestA + _ConstraintsTestB)()])
astropy/modeling/core.py:77: in _opfunc
    AstropyDeprecationWarning)
E   astropy.utils.exceptions.AstropyDeprecationWarning: Composition of model classes will be removed in 4.0(but composition of model instances is not affected)
_______________________________________________________ ERROR collecting astropy/tests/disable_internet.py ________________________________________________________
astropy/tests/disable_internet.py:25: in <module>
    "for more information.", AstropyDeprecationWarning)
E   astropy.utils.exceptions.AstropyDeprecationWarning: The ``disable_internet`` module is no longer provided by astropy. It is now available as ``pytest_remotedata.disable_internet``. However, developers are encouraged to avoid using this module directly. See <https://docs.astropy.org/en/latest/whatsnew/3.0.html#pytest-plugins> for more information.
________________________________________________________ ERROR collecting astropy/utils/compat/funcsigs.py ________________________________________________________
astropy/utils/compat/funcsigs.py:9: in <module>
    "use inspect instead", AstropyDeprecationWarning)
E   astropy.utils.exceptions.AstropyDeprecationWarning: astropy.utils.compat.funcsigs is now deprecated - use inspect instead
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 4 errors during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
=
@astrofrog

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

@bsipocz - I started doing this but it's a bit of a 🐰 🕳 as there are quite a few tests that use this, and I'm worried changes I make would conflict with @perrygreenfield's PR. Since we are going to remove this code anyway in master, I'd rather just let the tests emit the deprecation warnings since it isn't crucial.

@bsipocz

This comment has been minimized.

Copy link
Member

commented May 20, 2019

OK, fair enough. Bottom line there isn't that much more deprecated usage lurking around in the code.

@nden

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

This page has a section dedicated to combining model classes. Do you want to remove it here?

@pllim

pllim approved these changes May 20, 2019

@@ -72,7 +72,7 @@ def _opfunc(left, right):
# Deprecation is for https://github.com/astropy/astropy/issues/8234
if not (isinstance(left, Model) and isinstance(right, Model)):
warnings.warn(
'Composition of model classes will be removed in 4.0'
'Composition of model classes will be removed in 4.0 '

This comment was marked as outdated.

Copy link
@pllim

pllim May 20, 2019

Member

Trailing whitespace unnecessary.

Suggested change
'Composition of model classes will be removed in 4.0 '
'Composition of model classes will be removed in 4.0'

This comment has been minimized.

Copy link
@bsipocz

bsipocz May 20, 2019

Member

it is otherwise there is no space between this and text in next line

This comment has been minimized.

Copy link
@pllim

pllim May 20, 2019

Member

Ohh... okay. Don't mind me then. Sorry for the noise. LGTM, except I don't know if Tom R wanna also address #8736 (comment) here or not.

This comment has been minimized.

Copy link
@bsipocz

bsipocz May 20, 2019

Member

I suppose it makes sense to remove the narrative docs of a deprecated behaviour at the same time as the deprecation, otherwise it can be still picked up and used.

@astrofrog

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

Hmm it's not as simple as removing a section since the compound model instance section relies on the fact the compound model classes have been described first. It'll take me a couple of days to actually find the time to do this.

@pllim

This comment has been minimized.

Copy link
Member

commented May 20, 2019

@astrofrog , as stop gap, can we insert a .. warning:: statement about not using them until someone has time to remove the examples properly?

@nden

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

OK , then let's merge this as is. I'll work on a PR to reword the section about Compound models tomorrow. Is this still possible (not sure what the release status is)?

@pllim pllim merged commit e0e433f into astropy:master May 20, 2019

15 checks passed

astropy-bot:changelog Changelog entry not present, as expected since the **no-changelog-entry-needed** label is present
astropy-bot:milestone This pull request has a milestone set.
ci/circleci: 32bit Your tests passed on CircleCI!
Details
ci/circleci: egg-info-37 Your tests passed on CircleCI!
Details
ci/circleci: html-docs Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl202 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl212 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl222 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl300 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpldev Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing aa90993...86364a4
Details
codecov/project 86.95% remains the same compared to aa90993
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
giles Click details to preview the documentation build
Details

bsipocz added a commit that referenced this pull request May 27, 2019

Merge pull request #8736 from astrofrog/remove-compound-class
Remove deprecated use of class composition in modeling docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.