astropy / pyregion

ds9 region parser for python
https://pyregion.readthedocs.io
MIT License
39 stars 38 forks source link

Misc cleanup before 2.0 release #120

Closed cdeil closed 6 years ago

cdeil commented 6 years ago

I plan to do some misc fixes and cleanup here, before the 2.0 release.

Currently there are some fails in master: https://travis-ci.org/astropy/pyregion/builds/287512820

If there's anything difficult or controversial, I'll ask. No need to review at this point.

Work in progress ...

cdeil commented 6 years ago

@bsipocz - Do you remember what the fix was to suppress this list intersphinx issue? https://travis-ci.org/astropy/pyregion/jobs/287523737#L1007 I remember seeing it before, but can't remember or find it ...

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 63.504% when pulling 60bfe13e3fc3e2e373b4d68e3b9fb047147c82d3 on cdeil:cleanup into 7bde9dd1561c5b1a47b989a3a57232b6f15f7754 on astropy:master.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.04%) to 63.542% when pulling f5bf208e8f928f9055fa05364198d54a307f5008 on cdeil:cleanup into 7bde9dd1561c5b1a47b989a3a57232b6f15f7754 on astropy:master.

cdeil commented 6 years ago

Because of the list intersphinx issue, for now I've just allowed Sphinx warnings in the Python 2 docs build: 32fceb7 Very few people build the docs of this package anyways and there's really no reason why they should use Python 2 any more ... If someone wants to change back, sure, just go ahead with a commit in master or a pull request.

bsipocz commented 6 years ago

@cdeil - probably the simplest is to add them as exceptions to a docs/nitpick-exceptions file.

bsipocz commented 6 years ago

btw, I'm doing a new helpers release right now, if you want to included that here (though probably nothing major is in there that would affect pyregions).

cdeil commented 6 years ago

I don't think there's a reason to do a helpers update for this package.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.04%) to 63.542% when pulling 32fceb772c29351e775c892b73e512a12b26aded on cdeil:cleanup into 7bde9dd1561c5b1a47b989a3a57232b6f15f7754 on astropy:master.

cdeil commented 6 years ago

It looks like changes to PYTEST_HEADER_MODULES in conftest.py have no effect for this package? I'm always seeing the modules from Astropy core listed. The conftest.py file is executed, but changes to PYTEST_HEADER_MODULES have no effect on the prinout!??? @bsipocz - Can you reproduce? Do you have an idea what's wrong?

bsipocz commented 6 years ago

I don't think there's a reason to do a helpers update for this package.

at all? That case I can remove it from the auto-update list.

bsipocz commented 6 years ago

I'll check on the conftest stuff soon.

cdeil commented 6 years ago

I would suggest to keep this package on the astropy-helpers update list. I'm just saying if there are no known issues with what we have and CI is all green and this package is already at a recent astropy-helpers v2, then it's OK to release here and have other astropy-helpers updates and other fixes in future releases, no?

bsipocz commented 6 years ago

@cdeil - PYTEST_HEADER_MODULES seems to be working for me, could you give more details of what you see? Maybe are you on the astropy slack?

This is what I get:

================================================================================= test session starts ==================================================================================
platform darwin -- Python 3.5.4, pytest-3.2.2, py-1.4.34, pluggy-0.4.0

Running tests with pyregion version 2.0.dev320.
Running tests in lib.macosx-10.10-x86_64-3.5/pyregion docs.

Date: 2017-10-13T14:23:00

Platform: Darwin-14.5.0-x86_64-i386-64bit

Executable: /Users/bsipocz/.virtualenvs/astropy-dev/bin/python

Full Python Version: 
3.5.4 (default, Sep  8 2017, 18:45:31) 
[GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)]

encodings: sys: utf-8, locale: UTF-8, filesystem: utf-8
byteorder: little
float info: dig: 15, mant_dig: 15

Numpy: 1.13.1
Matplotlib: 2.0.2
Pandas: 0.20.3
Astropy: 3.0.dev20330
Using Astropy options: remote_data: none.

rootdir: /private/var/folders/dc/hsm7tqpx2d57n7vb3k1l81xw0000gq/T/pyregion-test-g4owfq37, inifile: setup.cfg
plugins: arraydiff-0.2.dev0, xdist-1.20.0, mpl-0.8, mock-1.6.2, forked-0.2, cov-2.5.1
collected 45 items                                                                                                                                                                      

pyregion/tests/test_cube.py .
pyregion/tests/test_ds9_attr_parser.py ...
pyregion/tests/test_ds9_region_parser.py ....
pyregion/tests/test_get_mask.py .
pyregion/tests/test_parser_helper.py .
pyregion/tests/test_region.py ......................
pyregion/tests/test_region_numbers.py .......
pyregion/tests/test_wcs.py ......
cdeil commented 6 years ago

I put this in conftest.py:

from collections import OrderedDict
PYTEST_HEADER_MODULES = OrderedDict([
    ('numpy', 'numpy'),
    ('matplotlib', 'matplotlib'),
    ('Astropy', 'astropy'),
    ('spam', 'spam'),
])

and I still get this, which I think is what Astropy has:

Numpy: 1.13.3
Scipy: 0.19.1
Matplotlib: 2.1.0
h5py: 2.7.0
Pandas: 0.20.3

Why?

bsipocz commented 6 years ago

It inherits the default from astropy, not sure what happens if you try to override that variable, it seems that nothing, you can't override it. But what I see on this branch is what we usually do, delete the keys that are unneeded, and add new ones. Is there a problem with that approach?

try:
    PYTEST_HEADER_MODULES['Astropy'] = 'astropy'
    del PYTEST_HEADER_MODULES['h5py']
    del PYTEST_HEADER_MODULES['Scipy']
except NameError:
    pass
cdeil commented 6 years ago

Is there a problem with that approach?

Yes, Astropy keeps getting new dependencies (like pandas now) that are not a dependency in affiliated packages. IMO it's simpler / cleaner to just have a separate / independent list here, instead of doing additions / deletions wrt Astropy core and having to change that over time.

Obviously this is not a big deal. If it's not clear how to do the list from scratch, I'll go back to the other scheme. @astrofrog - Maybe you have a tip?

bsipocz commented 6 years ago

OK, then it's really an issue for the package template. What I would do is to hack around with a list to keep and clear the dict automatically for everything else. Happy to come up with something after I see the helpers release out of the door.

bsipocz commented 6 years ago

Hmm, actually, this can be fixed in astropy. The dict is defined separately in the plugin, and there is indeed no reason to keep pandas or h5py or any astropy specific entry there.

I would suggest to keep numpy and mpl there, but even the latter could be removed from the default one.

cdeil commented 6 years ago

@bsipocz - I followed your suggestion to clear the dict and then fill it how I like. Works fine. Thanks!

Thinking about it a bit, it's not really surprising that making a new PYTEST_HEADER_MODULES didn't work. One has to change the existing dict that is already defined elsewhere and only brought into this namespace via the * import at the top.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.04%) to 63.542% when pulling a01d89e3669e615303a9d686781a36d04a172fad on cdeil:cleanup into 7bde9dd1561c5b1a47b989a3a57232b6f15f7754 on astropy:master.