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

Enabling raising deprecation warnings as exceptions #780

Closed
wants to merge 9 commits into from

Conversation

bsipocz
Copy link
Member

@bsipocz bsipocz commented Nov 17, 2016

This is to close #411.

While it passed locally I expect a few deprecation warnings to show up on travis.

@cdeil cdeil self-assigned this Nov 17, 2016
@cdeil cdeil added this to the 0.6 milestone Nov 17, 2016
@cdeil
Copy link
Contributor

cdeil commented Nov 17, 2016

I see these three issues on travis-ci:

I've assigned this to the Gammapy 0.6 milestone, i.e. not something we fully resolve now.

@bsipocz
Copy link
Member Author

bsipocz commented Nov 17, 2016

I hoped the ones in sherpa has already been gone away with new releases, but having only one from there is an improvement.

@bsipocz bsipocz force-pushed the conftest_enable_deprecations branch from 070d5fb to bc8fe04 Compare February 7, 2017 18:01
@bsipocz bsipocz modified the milestones: 0.7, 0.6 Feb 7, 2017
@bsipocz
Copy link
Member Author

bsipocz commented Feb 7, 2017

@cdeil - Moved this to 0.7. There are tons of warnings coming from astropy 1.3 (clobber vs overwrite) and I probably won't make a a 1.3.1 release before you release a 0.6.

@cdeil cdeil modified the milestones: 0.7, 0.8 Sep 14, 2017
@cdeil cdeil modified the milestones: 0.8, 0.9 May 3, 2018
@cdeil
Copy link
Contributor

cdeil commented Jun 19, 2018

@bsipocz - What should we do here?
Do you think this can / should go in?
Or will it always just remain an annoyance to have deprecation warnings come and break the CI build?

A compromise could be that we don't add this to CI, but add a step to the pre-release instructions to enable and check for this.

@bsipocz
Copy link
Member Author

bsipocz commented Jun 19, 2018

@cdeil - The machinery now is available to conveniently whitelist warnings that we don't have any control about, so I think this is a really good thing to have on CI so things are picked up sooner rather than later.

@cdeil cdeil modified the milestones: 0.9, 0.8 Jun 19, 2018
@bsipocz bsipocz force-pushed the conftest_enable_deprecations branch from bc8fe04 to 0e46290 Compare June 20, 2018 11:36
@bsipocz
Copy link
Member Author

bsipocz commented Jun 20, 2018

I've rebased to see how we stand, though I don't foresee to have any time right now to iron out things, feel free to push commits to this branch though.

@cdeil
Copy link
Contributor

cdeil commented Jul 6, 2018

Not sure what this deprecation warning is about or how to fix it:
https://travis-ci.org/gammapy/gammapy/jobs/394516123#L1957

@bsipocz bsipocz force-pushed the conftest_enable_deprecations branch from f88db73 to 0325976 Compare July 9, 2018 20:04
@bsipocz
Copy link
Member Author

bsipocz commented Jul 9, 2018

@cdeil - I've temporarily commented out the tests that causes weird errors during collection, see my comment here: https://github.com/gammapy/gammapy/pull/1387/files#r201127543

This case we can see the real deprecation warnings

@bsipocz bsipocz force-pushed the conftest_enable_deprecations branch from 445b147 to 1e3c3f9 Compare July 10, 2018 16:56
@bsipocz
Copy link
Member Author

bsipocz commented Jul 10, 2018

@cdeil - In the spirit of the gammapy sprint, I've cleaned up stuff for this PR.

Some things needed to be fixed in sherpa, so while PRs has been opened, I'll add one more commit for that here to temporarily avoid tripping over with those deprecations.

One failure is remaining locally (in addition to the weird fermi stuff in my comment above), that is very much looks unrelated:

============================================================= FAILURES ==============================================================
___________________________________________________________ test_cube_fit ___________________________________________________________

sky_model = SkyModel(spatial_model=<gammapy.image.models.new.SkyGaussian object at 0x10cd3a3c8>, spectral_model=PowerLaw())
counts = WcsNDMap

	geom      : Wcs 
 	unit      :  
	data shape: (9, 150, 400)
	data mean : 7.3e-03 
	data min  : -4.6e-16 
	data max  : 2.4e+00 

exposure = WcsNDMap

	geom      : Wcs 
 	unit      : m2 s 
	data shape: (9, 150, 400)
	data mean : 3.3e+09 m2 s
	data min  : 0.0e+00 m2 s
	data max  : 1.3e+10 m2 s

psf = <gammapy.cube.psf_kernel.PSFKernel object at 0x10c969c50>

    @requires_dependency('scipy')
    @requires_dependency('iminuit')
    @requires_data('gammapy-extra')
    def test_cube_fit(sky_model, counts, exposure, psf):
        input_model = sky_model.copy()
    
        input_model.parameters['lon_0'].value = 0
        input_model.parameters['index'].value = 2
        input_model.parameters['lat_0'].frozen = True
        input_model.parameters['sigma'].frozen = True
    
        input_model.parameters.set_parameter_errors({
            'lon_0': '0.1 deg',
            'index': '0.1',
            'amplitude': '1e-12 cm-2 s-1 TeV-1',
        })
    
        fit = SkyModelMapFit(model=input_model,
                             counts=counts,
                             exposure=exposure,
                             psf=psf)
        fit.fit()
    
        assert_quantity_allclose(fit.model.parameters['index'].quantity,
                                 sky_model.parameters['index'].quantity,
                                 rtol=1e-2)
        assert_quantity_allclose(fit.model.parameters['amplitude'].quantity,
                                 sky_model.parameters['amplitude'].quantity,
                                 rtol=1e-2)
        assert_quantity_allclose(fit.model.parameters['lon_0'].quantity,
                                 sky_model.parameters['lon_0'].quantity,
                                 rtol=1e-2)
    
        stat = np.sum(fit.stat, dtype='float64')
>       assert_allclose(stat, 8.672365798603572)
E       AssertionError: 
E       Not equal to tolerance rtol=1e-07, atol=0
E       
E       (mismatch 100.0%)
E        x: array(13869.181149)
E        y: array(8.672366)

gammapy/cube/tests/test_fit.py:119: AssertionError
------------------------------------------------------- Captured stdout call --------------------------------------------------------
**************************************************
*                     MIGRAD                     *
**************************************************

**********************************************************************
---------------------------------------------------------------------------------------
fval = 13869.128702669472 | total call = 91 | ncalls = 91
edm = 6.014690020777887e-06 (Goal: 1e-05) | up = 1.0
---------------------------------------------------------------------------------------
|          Valid |    Valid Param | Accurate Covar |         Posdef |    Made Posdef |
---------------------------------------------------------------------------------------
|           True |           True |           True |           True |          False |
---------------------------------------------------------------------------------------
|     Hesse Fail |        Has Cov |      Above EDM |                |  Reach calllim |
---------------------------------------------------------------------------------------
|          False |           True |          False |             '' |          False |
---------------------------------------------------------------------------------------

--------------------------------------------------------------------------------------------------
|      |   Name    |  Value   | Para Err |   Err-   |   Err+   |  Limit-  |  Limit+  |          |
--------------------------------------------------------------------------------------------------
|    0 |     lon_0 =  0.2     |  0.003343 |          |          |          |          |          |
|    1 |     lat_0 =  0.1     |  0       |          |          |          |          |  FIXED   |
|    2 |     sigma =  0.2     |  0       |          |          |          |          |  FIXED   |
|    3 |     index =  3       |  0.02084 |          |          |          |          |          |
|    4 | amplitude =  1E-11   |  3.35E-13 |          |          |          |          |          |
|    5 | reference =  1       |  0       |          |          |          |  0       |  FIXED   |
--------------------------------------------------------------------------------------------------

**********************************************************************
============================ 1 failed, 1264 passed, 55 skipped, 36 xfailed, 5 xpassed in 473.52 seconds =============================

@cdeil cdeil modified the milestones: 0.8, 0.9 Aug 14, 2018
@cdeil cdeil modified the milestones: 0.9, 0.10 Oct 31, 2018
@cdeil cdeil self-assigned this Jan 17, 2019
@cdeil cdeil modified the milestones: 0.10, 0.11 Jan 17, 2019
@cdeil cdeil added this to To Do in DOCUMENTATION via automation Jan 17, 2019
@cdeil
Copy link
Contributor

cdeil commented Mar 26, 2019

I'm closing this old PR.

I'm not sure if raising exceptions for deprecations is a good idea - they keep popping up and cause build fails in many unrelated PRs, no? Personally I'd prefer to avoid this and instead see and fix deprecation warnings locally every few months.

@bsipocz or @adonath - if you want to turn this on, OK.
But this PR has many merge conflicts, suggest you open a new one.

@cdeil cdeil closed this Mar 26, 2019
DOCUMENTATION automation moved this from To Do to Done Mar 26, 2019
@bsipocz
Copy link
Member Author

bsipocz commented Mar 26, 2019

@cdeil - It's your call, I'm happy either way. My experience is that having this one is less pain than not having it and running into issues along the line as keeping up the regular "fix deprecation warnings locally every few months" is super difficult.

@cdeil
Copy link
Contributor

cdeil commented Mar 27, 2019

@bsipocz - OK, maybe if it's possible to get green CI even with this on, then it might be a good idea to turn this on. Let's see what the various CI builds here say: #2091 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
DOCUMENTATION
  
Done
Development

Successfully merging this pull request may close these issues.

Turn back enable_deprecations_as_exceptions in conftest
2 participants