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

./bin/diagnose_imports: does it work at all?! #9276

Closed
skirpichev opened this Issue Apr 9, 2015 · 9 comments

Comments

Projects
None yet
2 participants
@skirpichev
Copy link
Contributor

skirpichev commented Apr 9, 2015

$ ./bin/test sympy/utilities/tests/test_module_imports.py 
============================================================================= test process starts ==============================================================================
executable:         /usr/bin/python  (2.7.3-final-0) [CPython]
architecture:       64-bit
cache:              yes
ground types:       gmpy 1.15
random seed:        95623160
hash randomization: on (PYTHONHASHSEED=3132452056)

sympy/utilities/tests/test_module_imports.py[1] X                     

Test is ok. Just as:

$ python ./bin/diagnose_imports --problems
$ # got nothing

But you have a lot of things like:

$ head -3 sympy/series/limits.py 
from __future__ import print_function, division

from sympy.core import S, Symbol, Add, sympify, Expr, PoleError, Mul, C  # <---!

I hope I misunderstand something, but according to the
docstring - this should be reported. Either diagnose_imports.py is broken and useless
or it's docs are misleading.

@toolforger

This comment has been minimized.

Copy link
Contributor

toolforger commented Apr 9, 2015

It never was really tested, and is being superseded anyway.

@skirpichev

This comment has been minimized.

Copy link
Contributor

skirpichev commented Apr 9, 2015

Perhaps, it should be removed?

@toolforger

This comment has been minimized.

Copy link
Contributor

toolforger commented Apr 10, 2015

Don't know what's wrong, the code used to spit out tons of errors (not the test code but bin/diagnose_imports).

Whether to keep it or not - meh.
Sure it could be removed, but
a) there are some much more pressing issue at hand (solvers)
b) as soon as #8909 is being worked on again it's nice to be able to look back at the design decisions as recorded in current master.
That said, I wouldn't object to removing the code, provided that bin/diagnose_imports is kept and contains a reference to the PR so people can easily find the old code once they decide they want to hack on it.

@skirpichev

This comment has been minimized.

Copy link
Contributor

skirpichev commented Apr 10, 2015

the code used to spit out tons of errors

I hope so. But it seems it doesn't work right now, see bugreport. I was wrong?

Sure it could be removed

Everything that doesn't work and isn't supported properly - should be removed in a live project.

@toolforger

This comment has been minimized.

Copy link
Contributor

toolforger commented Apr 10, 2015

@skirpichev

This comment has been minimized.

Copy link
Contributor

skirpichev commented Apr 10, 2015

If you don't think it's a bug - just close the bugreport...

@toolforger

This comment has been minimized.

Copy link
Contributor

toolforger commented Apr 10, 2015

@certik this is a duplicate of #8909, please close this.

@skirpichev

This comment has been minimized.

Copy link
Contributor

skirpichev commented Apr 10, 2015

#8909 - is not an issue. Does this pr fix mentioned issues?

a reference to the PR so people can easily find the old code once they decide they want to hack on it.

PR, even closed, are available to all. But if you do think that sympy master should be a placeholder for dead code...

(As a side note, you should try submit this code to some more suitable project - pylint or something. And definitely, you will get better and more helpful review.)

skirpichev added a commit to skirpichev/diofant that referenced this issue Nov 2, 2016

Add regression tests & mention closed issues
    close sympy/sympy#3112 (MrvAsympt was added in diofant#6)
    close sympy/sympy#9173 (test was added in 5a510ac)
    close sympy/sympy#9808 (fixed in 09e539b)
    close sympy/sympy#9341 (fixed in af98a00)
    close sympy/sympy#9908 (fixed in cc3fa8d)
    close sympy/sympy#6171 (test added in d278031)
    close sympy/sympy#9276 (diagnose_imports.py removed in ab8c535)
    close sympy/sympy#10201 (fixed in 0d0fc5f)
    close sympy/sympy#9057 (test was added in 8290a0c)
    close sympy/sympy#11159 (test was added in ffb76cb)
    close sympy/sympy#2839 (new AST transformers are used, see diofant#278 and diofant#167)
    close sympy/sympy#11081 (see ed01e16 and bb92329)
    close sympy/sympy#10974 (see 73fc425)
    close sympy/sympy#10806 (test in 539929a)
    close sympy/sympy#10801 (test in 2fe3da5)
    close sympy/sympy#9549 (test in 88bdefa)
    close sympy/sympy#4231 (test was added in fb411d5)
    close sympy/sympy#8634 (see 2fcbb58)
    close sympy/sympy#8481 (see 1ef20d3)
    close sympy/sympy#9956 (fixed in a34735f)
    close sympy/sympy#9747 (see e117c60)
    close sympy/sympy#7853 (see 3e4fbed)
    close sympy/sympy#9634 (see 2be03f5)
    close sympy/sympy#8500 (fixed in diofant#104 and finally in diofant#316)
    close sympy/sympy#9192 (see 9bf622f)
    close sympy/sympy#7130 (see e068fa3)
    close sympy/sympy#8514 (see b2d543b)
    close sympy/sympy#9334 (see 90de625)
    close sympy/sympy#8229 (see 9755b89)
    close sympy/sympy#8061 (see 7054f06)
    close sympy/sympy#7872 (tested in diofant#6)
    close sympy/sympy#3496 (tested in test_log_symbolic)
    close sympy/sympy#2929 (see da7db7a)
    close sympy/sympy#8203 (oo is not a real, see diofant#36)
    close sympy/sympy#7649 (0 is imaginary since diofant#8)
    close sympy/sympy#7256 (fixed in c0a4549)
    close sympy/sympy#6783 (see cb28d63)
    close sympy/sympy#5662 (is_integer issue fixed in 6bfa9f8, there is no is_bounded anymore)
    close sympy/sympy#5295 (fixed with diofant#354)
    close sympy/sympy#4856 (we now have flake/pep tests)
    close sympy/sympy#4555 (flake8 enabled after diofant#214)
    close sympy/sympy#5773 (cmp_to_key removed after diofant#164 and c9acbf0)
    close sympy/sympy#5484 (see above)

    Added regression tests:
    from https://groups.google.com/forum/#!topic/sympy/LkTMQKC_BOw
    fixes sympy/sympy#8825 (probably via diofant#209)
    fixes sympy/sympy#8635
    fixes sympy/sympy#8157
    fixes sympy/sympy#7872
    fixes sympy/sympy#7599
    fixes sympy/sympy#6179
    fixes sympy/sympy#5415
    fixes sympy/sympy#2865
    fixes sympy/sympy#5907
    fixes sympy/sympy#11722

    Closes diofant#347

skirpichev added a commit to skirpichev/diofant that referenced this issue Nov 2, 2016

Add regression tests & mention closed issues
    close sympy/sympy#3112 (MrvAsympt was added in diofant#6)
    close sympy/sympy#9173 (test was added in 5a510ac)
    close sympy/sympy#9808 (fixed in 09e539b)
    close sympy/sympy#9341 (fixed in af98a00)
    close sympy/sympy#9908 (fixed in cc3fa8d)
    close sympy/sympy#6171 (test added in d278031)
    close sympy/sympy#9276 (diagnose_imports.py removed in ab8c535)
    close sympy/sympy#10201 (fixed in 0d0fc5f)
    close sympy/sympy#9057 (test was added in 8290a0c)
    close sympy/sympy#11159 (test was added in ffb76cb)
    close sympy/sympy#2839 (new AST transformers are used, see diofant#278 and diofant#167)
    close sympy/sympy#11081 (see ed01e16 and bb92329)
    close sympy/sympy#10974 (see 73fc425)
    close sympy/sympy#10806 (test in 539929a)
    close sympy/sympy#10801 (test in 2fe3da5)
    close sympy/sympy#9549 (test in 88bdefa)
    close sympy/sympy#4231 (test was added in fb411d5)
    close sympy/sympy#8634 (see 2fcbb58)
    close sympy/sympy#8481 (see 1ef20d3)
    close sympy/sympy#9956 (fixed in a34735f)
    close sympy/sympy#9747 (see e117c60)
    close sympy/sympy#7853 (see 3e4fbed)
    close sympy/sympy#9634 (see 2be03f5)
    close sympy/sympy#8500 (fixed in diofant#104 and finally in diofant#316)
    close sympy/sympy#9192 (see 9bf622f)
    close sympy/sympy#7130 (see e068fa3)
    close sympy/sympy#8514 (see b2d543b)
    close sympy/sympy#9334 (see 90de625)
    close sympy/sympy#8229 (see 9755b89)
    close sympy/sympy#8061 (see 7054f06)
    close sympy/sympy#7872 (tested in diofant#6)
    close sympy/sympy#3496 (tested in test_log_symbolic)
    close sympy/sympy#2929 (see da7db7a)
    close sympy/sympy#8203 (oo is not a real, see diofant#36)
    close sympy/sympy#7649 (0 is imaginary since diofant#8)
    close sympy/sympy#7256 (fixed in c0a4549)
    close sympy/sympy#6783 (see cb28d63)
    close sympy/sympy#5662 (is_integer issue fixed in 6bfa9f8, there is no is_bounded anymore)
    close sympy/sympy#5295 (fixed with diofant#354)
    close sympy/sympy#4856 (we now have flake/pep tests)
    close sympy/sympy#4555 (flake8 enabled after diofant#214)
    close sympy/sympy#5773 (cmp_to_key removed after diofant#164 and c9acbf0)
    close sympy/sympy#5484 (see above)

    Added regression tests:
    from https://groups.google.com/forum/#!topic/sympy/LkTMQKC_BOw
    fixes sympy/sympy#8825 (probably via diofant#209)
    fixes sympy/sympy#8635
    fixes sympy/sympy#8157
    fixes sympy/sympy#7872
    fixes sympy/sympy#7599
    fixes sympy/sympy#6179
    fixes sympy/sympy#5415
    fixes sympy/sympy#2865
    fixes sympy/sympy#5907
    fixes sympy/sympy#11722

    Closes diofant#347
@skirpichev

This comment has been minimized.

Copy link
Contributor

skirpichev commented Jul 9, 2017

No activity, closed. Apparently, this is not an issue for the diofant.

@skirpichev skirpichev closed this Jul 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment