Sort imports by Cadair · Pull Request #7827 · astropy/astropy

Cadair · 2018-09-17T17:04:27Z

You don't understand, I sorted all imports... everywhere.

astropy-bot · 2018-09-17T17:04:33Z

Hi there @Cadair 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

bsipocz · 2018-09-17T17:39:00Z

While I truly love the cleanup this PR provides, we need to be careful when/how to merge as this will cause a hell lot of conflicts with other PRs.

Cadair · 2018-09-17T17:44:22Z

yes, yes it would.

astrofrog · 2018-09-17T19:15:49Z

-import pytest
 import numpy as np
+import pytest
 from numpy.testing import assert_allclose


Just to be difficult: I'm not a fan of this sorting, as I like to keep the Numpy imports together :)

I can add numpy as it's own section if you want?

astrofrog · 2018-09-17T19:17:33Z

Note that there are some places where the order of the imports is related to performance - I know @mhvk ordered some imports like that recently.

astrofrog · 2018-09-17T19:18:13Z

Bonus points if you can write a script that figures out if there would be any merge conflicts with any PRs automatically and avoids those files. Should be feasible, right?

bsipocz · 2018-09-17T19:18:45Z

Note that there are some places where the order of the imports is related to performance - I know @mhvk ordered some imports like that recently.

Once CI passed, I think then we should definitely need to run this PR through the benchmarks.

astrofrog · 2018-09-17T20:05:45Z

It is pretty easy to find a list of all files touched by open PRs:

from github import Github

gh = Github('<token>')
repo = gh.get_repo('astropy/astropy')
files = []
for pr in repo.get_pulls():
    if pr.number != 7827:  # this PR!
        for file in pr.get_files():
            files.append(file.filename)
print(list(set(files)))

['astropy/table/column.py', 'astropy/units/decorators.py', '.travis.yml', 'astropy/tests/runner.py', 'astropy/uncertainty/tests/test_uquantity.py', 'astropy/coordinates/builtin_frames/cirs_observed_transforms.py', 'astropy/convolution/tests/test_convolve_speeds.py', 'setup.py', 'astropy/utils/codegen.py', 'CODE_OF_CONDUCT.md', 'astropy/convolution/src/boundary_padded.c', 'astropy/modeling/tests/test_fitters.py', 'astropy/units/__init__.py', 'astropy/modeling/tests/test_models.py', 'astropy/table/tests/test_mixin.py', 'astropy/units/quantity_helper/erfa.py', 'astropy/nddata/mixins/tests/test_ndslicing.py', 'astropy/wcs/tests/test_fitswcs_low_level_api.py', 'astropy/convolution/boundary_none.pyx', 'astropy/utils/openmp.py', 'docs/development/workflow/get_devel_version.rst', 'astropy/modeling/parameters.py', 'astropy/table/groups.py', 'astropy/nddata/tests/test_nduncertainty.py', 'astropy/wcs/utils.py', 'astropy/coordinates/transformations.py', 'astropy/time/core.py', 'astropy/wcs/wcsapi/ucds.txt', 'astropy/tests/plugins/config.py', 'astropy/table/pprint.py', 'astropy/convolution/setup_package.py', 'astropy/units/core.py', 'astropy/coordinates/builtin_frames/skyoffset.py', 'astropy/time/tests/test_basic.py', 'astropy/wcs/__init__.py', 'astropy/coordinates/builtin_frames/icrs_cirs_transforms.py', 'astropy/modeling/separable.py', 'astropy/coordinates/angles.py', 'astropy/convolution/kernels.py', 'astropy/visualization/wcsaxes/core.py', 'astropy/nddata/nduncertainty.py', 'astropy/io/fits/hdu/hdulist.py', 'astropy/coordinates/tests/test_frames.py', 'astropy/units/tests/test_units.py', 'astropy/convolution/src/convolve.h', 'astropy/units/unitsystem.py', 'astropy/wcs/tests/test_base_low_level_api.py', 'astropy/units/quantity.py', 'astropy/table/tests/test_column.py', 'astropy/io/fits/hdu/table.py', 'astropy/coordinates/astrom_manager.py', 'docs/units/quantity.rst', 'astropy/uncertainty/tests/__init__.py', 'astropy/io/misc/hdf5.py', 'astropy/wcs/setup_package.py', 'astropy/tests/coveragerc', 'astropy/uncertainty/tests/test_variable.py', 'docs/Makefile', 'astropy/wcs/wcsapi/tests/__init__.py', 'astropy/visualization/mpl_normalize.py', 'astropy/io/ascii/tests/test_write.py', 'astropy/coordinates/baseframe.py', 'docs/visualization/normalization.rst', 'astropy/nddata/nddata.py', 'astropy/wcs/wcsapi/tests/test_base_low_level_api.py', 'astropy/modeling/tests/test_compound.py', 'docs/coordinates/performance.inc.rst', 'docs/conf.py', 'astropy/io/ascii/fastbasic.py', 'astropy/coordinates/sky_coordinate.py', 'astropy/wcs/base_low_level_api.py', 'astropy/convolution/convolve.py', 'astropy/utils/data_info.py', 'astropy/coordinates/tests/test_finite_difference_velocities.py', 'astropy/convolution/boundary_fill.pyx', 'astropy/table/__init__.py', 'astropy/units/quantity_helper/scipy_special.py', 'astropy/io/fits/connect.py', 'astropy/modeling/fitting.py', 'astropy/coordinates/angle_utilities.py', 'astropy/io/fits/column.py', 'astropy/wcs/fitswcs_low_level_api.py', 'astropy/wcs/wcsapi/__init__.py', 'astropy/modeling/optimizers.py', 'astropy/nddata/mixins/tests/test_ndarithmetic.py', 'astropy/modeling/core.py', 'astropy/wcs/ucds.txt', 'docs/nddata/nddata.rst', 'astropy/wcs/wcsapi/base_low_level_api.py', 'astropy/units/quantity_helper/converters.py', 'astropy/wcs/wcs.py', 'astropy/tests/pytest_plugins.py', 'astropy/visualization/tests/test_norm.py', 'astropy/convolution/utils.py', 'astropy/wcs/tests/test_high_level_api.py', 'astropy/table/serialize.py', 'docs/nddata/mixins/ndarithmetic.rst', 'CHANGES.rst', 'astropy/units/tests/test_quantity.py', 'docs/modeling/units.rst', 'astropy/samp/hub_script.py', 'astropy/io/ascii/ecsv.py', 'astropy/units/tests/test_unitsystem.py', 'astropy/utils/tests/test_decorators.py', 'docs/units/performance.inc.rst', 'astropy/time/formats.py', 'astropy/stats/radial_profile.py', 'astropy/units/tests/test_quantity_ufuncs.py', 'astropy/coordinates/tests/test_sky_coord.py', 'astropy/uncertainty/uquantity.py', 'astropy/nddata/compat.py', 'astropy/units/quantity_helper/helpers.py', 'astropy/nddata/mixins/ndslicing.py', 'astropy/units/quantity_helper.py', 'astropy/utils/decorators.py', 'astropy/wcs/high_level_api.py', 'astropy/units/quantity_helper/__init__.py', 'docs/io/fits/index.rst', 'astropy/uncertainty/__init__.py', 'astropy/modeling/tests/test_separable.py', 'astropy/units/utils.py', 'astropy/io/fits/tests/test_hdulist.py', 'docs/spelling_wordlist.txt', 'astropy/uncertainty/core.py', 'astropy/convolution/boundary_extend.pyx', 'astropy/nddata/tests/test_nddata.py', 'astropy/wcs/tests/test_wcs.py', 'astropy/coordinates/representation.py', 'astropy/convolution/boundary_wrap.pyx', 'astropy/modeling/functional_models.py', 'astropy/convolution/src/boundary_none.c', 'astropy/units/tests/test_quantity_decorator.py', 'astropy/io/misc/tests/test_hdf5.py']

It might make sense to try and run this only for untouched files to start with? Btw this could be helpful for PEP8 cleanup too.

astrofrog · 2018-09-17T21:18:13Z

@bsipocz - it's not just about existing benchmarks, but to do with the timing of astropy sub-package imports, which we don't test automatically.

bsipocz · 2018-09-17T21:21:13Z

@astrofrog - Of course, you're right, we need those too. So apparently running the tools is the easy part of the job 😁

mhvk · 2018-09-17T21:22:23Z

I don't think anything I did to reduce import times of modules is changed by this PR - at least, if I understand correctly, it is just organizing the imports on-top, while my changes moved imports from on-top to inline, etc.

bsipocz · 2018-09-17T21:48:29Z

+from numpy.testing import assert_allclose, assert_almost_equal

 from ..utils import KernelSizeError
+from ..kernels import (Kernel1D, Kernel2D, Box1DKernel, Box2DKernel, CustomKernel,


this must be beyond 80 characters while most of this file is within

I set the line limit to 99, does astropy use 80 everywhere?

it depends on the module, some are 80 some are 100.

in fact all the other tools in setup.cfg are configured to 100 lines.

http://docs.astropy.org/en/stable/development/codeguide.html#coding-style-conventions

It's set to 100 so it won't complain for modules where the limit is not 80.

yeah it's one or the other ;)

We had this discussion quite a while back, and it was decided one should follow the style of the module - PEP8 states it should be 80, so I think 100 should be the exception, not the rule.

Why not 79? We need that 80th char for \0. 😄

saimn · 2018-09-18T07:50:05Z

I'm not convinced by this import length sort, but hey you British folks like weird units .
This can also cause issues with circular imports, I guess this is why CI are failing!

bsipocz · 2018-09-18T07:58:44Z

weird units

Oh, those units are not seriously used in real life. On the other hand, the situation on the other side of the pond is complete madness, so many of the EU regulations were not the doing of the devil's work

pllim · 2018-09-18T14:31:21Z

the situation on the other side of the pond is complete madness

Haha. Welcome to USA!

pllim added this to the v3.1 milestone Sep 17, 2018

pllim added Refactoring no-changelog-entry-needed labels Sep 17, 2018

Cadair added 2 commits September 17, 2018 18:06

Add isort config

88b423e

run isort

ea23e5e

Cadair force-pushed the isort branch from 91ac29b to ea23e5e Compare September 17, 2018 17:07

Cadair mentioned this pull request Sep 17, 2018

MNT: Remove unused imports from code base #7824

Closed

astrofrog reviewed Sep 17, 2018

View reviewed changes

bsipocz reviewed Sep 17, 2018

View reviewed changes

Cadair closed this Oct 11, 2018

bsipocz mentioned this pull request Nov 30, 2018

Change all relative imports to absolute imports (except same-module imports) #8200

Merged

Cadair mentioned this pull request Dec 6, 2018

Add a config for isort #8235

Merged

Conversation

Cadair commented Sep 17, 2018

Uh oh!

astropy-bot Bot commented Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsipocz commented Sep 17, 2018

Uh oh!

Cadair commented Sep 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astrofrog commented Sep 17, 2018

Uh oh!

astrofrog commented Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsipocz commented Sep 17, 2018

Uh oh!

astrofrog commented Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astrofrog commented Sep 17, 2018

Uh oh!

bsipocz commented Sep 17, 2018

Uh oh!

mhvk commented Sep 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saimn commented Sep 18, 2018

Uh oh!

bsipocz commented Sep 18, 2018

Uh oh!

pllim commented Sep 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

astropy-bot Bot commented Sep 17, 2018 •

edited

Loading

astrofrog commented Sep 17, 2018 •

edited

Loading

astrofrog commented Sep 17, 2018 •

edited

Loading