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

Completely untested modules in sympy.polys #5470

Closed
rlamy opened this issue May 14, 2011 · 15 comments
Closed

Completely untested modules in sympy.polys #5470

rlamy opened this issue May 14, 2011 · 15 comments

Comments

@rlamy
Copy link
Member

rlamy commented May 14, 2011

The modules densepolys, sparsearith, sparsebasic, sparsepolys and sparsetools have 0% coverage by the test suite, which means that they aren't even imported. They don't have any doctest either.

Quite unsurprisingly, they don't seem to work correctly:

In [1]: from sympy.polys.densepolys import DensePoly

In [2]: DensePoly(x, x)
---------------------------------------------------------------------------
NameError                                 Traceback (most recent call last)

/media/sda2/Boulot/Projets/sympy-git/<ipython console> in <module>()

/media/sda2/Boulot/Projets/sympy-git/sympy/polys/densepolys.pyc in __init__(self, rep, dom, lev)
     10     def __init__(self, rep, dom, lev=None):
     11         if lev is None:
---> 12             rep, lev = dmp_validate(rep)
     13 
     14         self.rep = rep

NameError: global name 'dmp_validate' is not defined

In [3]: from sympy.polys.sparsepolys import SparsePoly

In [4]: SparsePoly(x, x)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

/media/sda2/Boulot/Projets/sympy-git/<ipython console> in <module>()

TypeError: __init__() takes at least 4 arguments (3 given)


So they should either be fixed, documented and tested before the release, or be removed entirely (since they aren't used at all, they're probably not that useful).

Original issue for #5470: http://code.google.com/p/sympy/issues/detail?id=2371
Original author: https://code.google.com/u/101272611947379421629/
Original owner: https://code.google.com/u/101069955704897915480/

@asmeurer
Copy link
Member

I think at least some of this (like the sparse stuff) is WIP code that isn't finished yet.

**Cc:** matt...@gmail.com  

Original comment: http://code.google.com/p/sympy/issues/detail?id=2371#c1
Original author: https://code.google.com/u/asmeurer@gmail.com/

@asmeurer
Copy link
Member

**Labels:** Polynomial  

Original comment: http://code.google.com/p/sympy/issues/detail?id=2371#c2
Original author: https://code.google.com/u/asmeurer@gmail.com/

@asmeurer
Copy link
Member

These are not imported into the global namespace with "from sympy import *", so I don't see too much danger in leaving them.  I am pretty sure that it is just WIP code.

Mateusz, did you want to work on this?  I am postponing otherwise.

**Labels:** -Milestone-Release0.7.0 Milestone-Release0.7.1  

Original comment: http://code.google.com/p/sympy/issues/detail?id=2371#c3
Original author: https://code.google.com/u/asmeurer@gmail.com/

@mattpap
Copy link
Member

mattpap commented May 18, 2011

This code will replace polyclasses.py (there is even preliminary support for this in polyoptions.py). I'm fine with postponing, because this will require still some work and it will be convenient to work on this in parallel with merge of lpoly1.

**Owner:** matt...@gmail.com  
**Cc:** -matt...@gmail.com  

Original comment: http://code.google.com/p/sympy/issues/detail?id=2371#c4
Original author: https://code.google.com/u/101069955704897915480/

@rlamy
Copy link
Member Author

rlamy commented May 27, 2011

This is public code, since it's accessible via public names, and the Repr option is actually imported but can't work. I really think we shouldn't release broken code if we can avoid it. It's easy to remove this now and it'll be easy to add it back in: https://github.com/sympy/sympy/pull/372

**Labels:** NeedsReview  

Original comment: http://code.google.com/p/sympy/issues/detail?id=2371#c5
Original author: https://code.google.com/u/101272611947379421629/

@asmeurer
Copy link
Member

I'd like to do a small 0.7.1 release with IPython 0.11 support, so these will be postponed until 0.7.2.

**Labels:** Milestone-Release0.7.2  

Original comment: http://code.google.com/p/sympy/issues/detail?id=2371#c6
Original author: https://code.google.com/u/asmeurer@gmail.com/

@rlamy
Copy link
Member Author

rlamy commented Nov 7, 2011

Well we released with these broken modules, so this is moot.

**Status:** WontFix  

Original comment: http://code.google.com/p/sympy/issues/detail?id=2371#c7
Original author: https://code.google.com/u/101272611947379421629/

@vks
Copy link
Contributor

vks commented Nov 15, 2011

I think we should still fix them and add tests, or remove them.

**Status:** Verified  

Original comment: http://code.google.com/p/sympy/issues/detail?id=2371#c8
Original author: https://code.google.com/u/Vinzent.Steinberg@gmail.com/

@rlamy
Copy link
Member Author

rlamy commented Nov 16, 2011

I think you mean "Accepted", not "Verified". The last time, Aaron and Mateusz thought it was useful to release broken code, but I guess we could reopen the discussion for the upcoming release. I still don't see what we lose in removing them (isn't it the whole point of using a VCS that it's easy to go back to a previous version of the code?) and they do cause problems, such as causing a ton of warnings in pyflakes (e.g. when running './setup.py audit').

**Status:** NeedsDecision  
**Labels:** -NeedsReview  

Original comment: http://code.google.com/p/sympy/issues/detail?id=2371#c9
Original author: https://code.google.com/u/101272611947379421629/

@rlamy
Copy link
Member Author

rlamy commented Nov 16, 2011

**Blocking:** 4555  

Referenced issues: #4555
Original comment: http://code.google.com/p/sympy/issues/detail?id=2371#c10
Original author: https://code.google.com/u/101272611947379421629/

@asmeurer
Copy link
Member

**Blocking:** 5958  

Referenced issues: #5958
Original comment: http://code.google.com/p/sympy/issues/detail?id=2371#c11
Original author: https://code.google.com/u/asmeurer@gmail.com/

@sympy-issue-migrator
Copy link

I'm running into exactly the same kind of problem as Ronan: I'm getting lots of distracting unhelpful diagnostics about code that isn't finished enough that it's even supposed to work. densepolys.py alone is spamming me with over 100 errors (missing dmp_ functions) and another 100 warnings (first parameter of class functions should be named "self").

I second removing them from the 'master' branch, until they have matured enough to be reintegrated again.
If we really want this code in the official codebase, create an 'untested' branch and move the code there.

Original comment: http://code.google.com/p/sympy/issues/detail?id=2371#c12
Original author: https://code.google.com/u/103802103633477003048/

@rlamy
Copy link
Member Author

rlamy commented May 8, 2012

Second attempt: https://github.com/sympy/sympy/pull/1283

**Labels:** NeedsReview  

Original comment: http://code.google.com/p/sympy/issues/detail?id=2371#c13
Original author: https://code.google.com/u/101272611947379421629/

@smichr
Copy link
Member

smichr commented May 8, 2012

OK, Ronan, I have now read here the rational for removing the things that you removed. I am in agreement that until they are mature enough to include they should not be included.

Original comment: http://code.google.com/p/sympy/issues/detail?id=2371#c14
Original author: https://code.google.com/u/117933771799683895267/

@smichr
Copy link
Member

smichr commented May 10, 2012

They are now removed.

**Status:** Fixed  

Original comment: http://code.google.com/p/sympy/issues/detail?id=2371#c15
Original author: https://code.google.com/u/117933771799683895267/

@jrioux
Copy link
Member

jrioux commented Sep 25, 2012

**Blocking:** -sympy:1456 -sympy:2859 sympy:1456 sympy:2859 sympy:1456  

Original comment: http://code.google.com/p/sympy/issues/detail?id=2371#c16
Original author: https://code.google.com/u/102137482174297837682/

@rlamy rlamy added this to the Release0.7.2 milestone Mar 7, 2014
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants