Refactoring the old assumptions #1162

Merged
merged 45 commits into from May 7, 2012

Projects

None yet

6 participants

@rlamy
Member
rlamy commented Mar 22, 2012

Here's a major refactoring of the old assumption system. This amounts to a complete rewrite of sympy.core.assumptions and of a large part of sympy.core.facts. The important changes are:

  • assumptions are now stored in a custom dict subclass
  • Basic doesn't accept assumption kwargs any more, only Symbol and its subclasses do.
  • For non-Symbols, "assumptions" are now best thought of as a property cache.
  • For Symbols, assumptions are a static dict computed upon instance creation.
  • Pickling has been refactored. It's now independent of __slots__ and all protocols behave like protocol 2.
  • Instance-level caches (e.g. _assumptions, _mhash) aren't pickled any more, which makes (some/most) pickles several times smaller.
  • AssumeMixin is gone. (I'd hoped it would allow jython to work again, but it just doesn't like mixing __slots__ and multiple inheritance)

I guess this'll be a bit painful to review, so thanks in advance. I've tried to keep commits small and focused in the hope that most of them can be understood without knowing the big picture.

@smichr
Member
smichr commented Mar 22, 2012

It's taken almost two weeks to hang up the hat for me, but I can work through this steadily a little every day. Just so I understand...where does this put us in making the shift from old to new assumptions?

@rlamy
Member
rlamy commented Mar 22, 2012

Good question... Mostly, I'd say that it reduces the entanglement of the old assumption system with the rest of the core and breaks it into smaller chunks. For non-Symbols, hashing and pickling are now independent of assumptions and the hairy metaprogramming stuff is confined to the PropertyManager object and the ManagedProperties metaclass. For Symbols, hashing and pickling still involve assumptions (there's no way around it, because assumptions are part of the object's definition), but the implementation of assumptions is simpler (just a dict lookup).

@rlamy
Member
rlamy commented Mar 22, 2012

SymPy Bot Summary: There were test failures.

@rlamy: Please fix the test failures.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYmJMRDA

Interpreter: /usr/bin/python3 (3.2.2-final-0)
Architecture: Linux (32-bit)
Cache: yes
Test command: setup.py test
master hash: c6a7d9d
branch hash: bf2bc368ee6f60462812e28fbb86ab926a1b93f5

Automatic review by SymPy Bot.

@rlamy
Member
rlamy commented Mar 22, 2012

Rebased to fix a stupid py3k mistake.

@rlamy
Member
rlamy commented Mar 22, 2012

SymPy Bot Summary: All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY_poRDA

Interpreter: /usr/bin/python3 (3.2.2-final-0)
Architecture: Linux (32-bit)
Cache: yes
Test command: setup.py test
master hash: c6a7d9d
branch hash: 95c7d4e

Automatic review by SymPy Bot.

@asmeurer
Member

As I said on the list, I can't say much about the changes to the logic code (maybe @haz or someone else who knows about that can review it). I'll take a look at the rest, though.

Based on what you said about pickling in the OP, that sounds good. I wasn't really a fan of the way pickling worked with __slots__. Does this fix issue 2587? What other issues are fixed here?

If you have a better idea of what's going on with the Jython issue, it would be great if you could continue the discussion at http://bugs.jython.org/issue1777. The Jython devs seem to think that what we are doing with __slots__ is a hack, which I'm not at all convinced of, but I still am quite shaky on how that very complicated part of Python works, so I can't really say for sure. Also, I have a feeling that you would have an easier time tracking down that latest bug than I would (or is that not even relevant any more with this PR?).

@asmeurer asmeurer commented on the diff Mar 23, 2012
sympy/core/basic.py
def __hash__(self):
# hash cannot be cached using cache_it because infinite recurrence
# occurs as hash is needed for setting cache dictionary keys
h = self._mhash
if h is None:
- h = (type(self).__name__,) + self._hashable_content()
-
- if self._assume_type_keys is not None:
- a = []
- kv= self._assumptions
- for k in sorted(self._assume_type_keys):
- a.append( (k, kv[k]) )
-
- h = hash( h + tuple(a) )
-
- else:
- h = hash( h )
-
-
+ h = hash((type(self).__name__,) + self._hashable_content())
@asmeurer
asmeurer Mar 23, 2012 Member

I still think that hashable_content should just include the name, but that's auxiliary to this PR.

@asmeurer
Member

I suppose some speed analysis should be done, since this modifies the core pretty heavily.

@asmeurer
Member

So with this new code, is it possible to differentiate between set assumptions and deduced ones? I did x = Symbol('x', positive=True), but I couldn't find a way to determine that I had done that and not, e.g., x = Symbol('x', nonnegative=True, zero=False).

@asmeurer
Member

So as far as I can tell, there isn't. The pickling is now better, but wouldn't it be better to only pickle those assumptions that were given, so that you only pickle, e.g., {'positive': True} instead of {'nonzero': True, 'real': True, 'commutative': True, 'nonpositive': False, 'positive': True, 'negative': False, 'nonnegative': True, 'zero': False, 'complex': True, 'imaginary': False}? Or would that be too much work?

@asmeurer asmeurer commented on an outdated diff Mar 23, 2012
sympy/core/assumptions.py
@@ -1,4 +1,50 @@
-from sympy.core.facts import FactRules
+"""
+This module contains the machinery handling assumptions.
+
+All symbolic objects have assumption attributes that can be accessed via
+.is_<assumption name> attribute.
+
+Assumptions determine certain properties of symbolic objects. Assumptions
+can have 3 possible values: True, False, None. None is returned when it is
+impossible to say something about the property. For example, a Symbol is
+not know beforehand to be positive.
@asmeurer
asmeurer Mar 23, 2012 Member

know => known

@asmeurer asmeurer commented on an outdated diff Mar 23, 2012
sympy/core/assumptions.py
@@ -1,4 +1,50 @@
-from sympy.core.facts import FactRules
+"""
+This module contains the machinery handling assumptions.
+
+All symbolic objects have assumption attributes that can be accessed via
+.is_<assumption name> attribute.
+
+Assumptions determine certain properties of symbolic objects. Assumptions
+can have 3 possible values: True, False, None. None is returned when it is
+impossible to say something about the property. For example, a Symbol is
@asmeurer
asmeurer Mar 23, 2012 Member

Maybe better to say "a generic Symbol".

@asmeurer asmeurer commented on the diff Mar 23, 2012
sympy/core/assumptions.py
+without specifying the property. For example, a symbol that has a property
+being integer, is also real, complex, etc.
+
+Here follows a list of possible assumption names:
+
+ - commutative - object commutes with any other object with
+ respect to multiplication operation.
+ - real - object can have only values from the set
+ of real numbers
+ - integer - object can have only values from the set
+ of integers
+ - bounded - object absolute value is bounded
+ - positive - object can have only positive values
+ - negative - object can have only negative values
+ - nonpositive - object can have only nonpositive values
+ - nonnegative - object can have only nonnegative values
@asmeurer
asmeurer Mar 23, 2012 Member

Why are these indented more?

@asmeurer
asmeurer Mar 23, 2012 Member

Oh, I guess you just moved this.

@asmeurer
Member

OK, I reviewed through this, and just have those comments. As I noted, I didn't really look at the logic stuff, as I have no idea what's going on there. There are a few comments that should be addressed, and some questions I'd like answered, but in general, this looks pretty good.

@haz
Contributor
haz commented Mar 23, 2012

I'm afraid I can't be much help in reviewing the logic stuff: it isn't hooked up to the new reasoning that is in place (the stuff that I worked on). Rather it uses the old logical deduction rules.

This will cause some inferences to not be found, even if we have enough information to do so (that is, not all logical inferences will be captured). That said, the change to the interface for assumptions is a good step towards that ultimate goal. The criticism(s) against computing all inferences at instantiation time of an object will likely need to be ignored until a proper inference engine is plugged in (at which point they should only be computed lazily once queried).

@rlamy
Member
rlamy commented Mar 23, 2012

Pickling: this fixes issues 2204 and 2587, but nothing else, I think (e.g. not 1198 or 3130). The polys classes could probably be refactored in the same way, but it's not clear that all of them need __slots__.

Symbol instantiation: the assumptions need to be fully worked out for canonicalisation, i.e. so that Symbol('x', positive=True) == Symbol('x', nonnegative=True, zero=False). Storing the assumptions that were set explicitly would be inconsistent with this equality, and inverting the deduction process to get back to a minimal set of assumptions is hard.

jython and __slots__: basically, any use of __slots__ is a hack, and jython's handling of it is broken (e.g. the bug you mention on Jython's tracker is also slots-related). After this PR, __slots__ can probably be removed without breaking much, ... which reveals other jython bugs, like http://bugs.jython.org/issue1676

Speed: I haven't noticed any obvious impact, and I didn't expect any either, since I haven't changed much to the runtime logic of assumption handling, and also since that doesn't take up a lot of time anyway, compared to everything else (expand, Mul.flatten, ...)

@asmeurer
Member

Storing the assumptions that were set explicitly would be inconsistent with this equality, and inverting the deduction process to get back to a minimal set of assumptions is hard.

I was thinking to store _set_assumptions, but compute the assumptions in _assumptions as we do now, and use that for comparison. Then we can just pickle _set_assumptions, and recompute _assumptions. I suppose we should weigh speed vs. space in this case, but if we cache assumptions, I imagine it would not be too slow. I'm mainly concerned about space for pickling because in SymPy Live, we are limited in the size of expression we can compute with due to pickle size (and the current upper bound size is not really that large). Not storing None or any non-Symbol assumptions should make a big difference here, but I think we can do even better.

This would also let us do things like http://code.google.com/p/sympy/issues/detail?id=3032 better (srepr(Symbol('x', positive=True) could give "Symbol('x', positive=True)" instead of "Symbol('x', nonzero=True, real=True, commutative=True, nonpositive=False, positive=True, negative=False, nonnegative=True, zero=False, complex=True, imaginary=False)").

And wouldn't this also make fixing issue 3130 easier?

Pickling: this fixes issues 2204 and 2587, but nothing else

I actually meant all issues, not just pickling related ones.

Speed: I haven't noticed any obvious impact, and I didn't expect any either, since I haven't changed much to the runtime logic of assumption handling, and also since that doesn't take up a lot of time anyway, compared to everything else (expand, Mul.flatten, ...)

Well, you've modified __slots__ for example, which I know can affect performance. I don't really have doubts about the performance of this branch, I'd just like to see some benchmarks to be sure.

jython and slots: basically, any use of slots is a hack, and jython's handling of it is broken (e.g. the bug you mention on Jython's tracker is also slots-related). After this PR, slots can probably be removed without breaking much, ... which reveals other jython bugs, like http://bugs.jython.org/issue1676

Well I'm not too hopeful that this will work any time soon then.

By the way, isn't it better to use __slots__ when you can, because it significantly reduces the memory footprint?

@rlamy
Member
rlamy commented Mar 26, 2012

And wouldn't this also make fixing issue 3130 easier?

What you're suggesting would reduce the size of the assumptions dict, but the only thing that matters for 3130 is that this dict can't be passed to __new__.

I actually meant all issues, not just pickling related ones.

I don't think this solves any other issue, except perhaps 1612 but only when the cache is on.

By the way, isn't it better to use __slots__ when you can, because it significantly reduces the memory footprint?

Using __slots__ cripples Python's object model and doesn't save much memory in our case, since we store a lot of informations on every instance. It also forces some rather bad design with subclasses being forced to have irrelevant private attributes, e.g. Symbol._args.

@asmeurer
Member
asmeurer commented Apr 5, 2012

What you're suggesting would reduce the size of the assumptions dict, but the only thing that matters for 3130 is that this dict can't be passed to new.

Ah, I didn't realize this. I guess we'll have to wait for pickle protocol 4 (it's on the PSF GSoC ideas list for this year, so maybe someone will do it). We could probably do some hackish workaround by injecting it in .args, but it's probably better to just wait for the new assumptions and figure things out from there.

I don't think this solves any other issue, except perhaps 1612 but only when the cache is on.

I thought that issue only showed up with the cache on.

Using slots cripples Python's object model and doesn't save much memory in our case, since we store a lot of informations on every instance.

Doesn't __slots__ also make things faster? I seem to remember that, though I could be wrong (I do remember I test removed __slots__ from a major class in the core to try to fix the Jython thing, and the tests ran several times slower).

@Krastanov
Member

SymPy Bot Summary: ❗️ There were merge conflicts; could not test the branch.

@rlamy: Please rebase or merge your branch with master. See the report for a list of the merge conflicts.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYtLQTDA

Interpreter: /usr/bin/python2.6 (2.6.7-final-0)
Architecture: Linux (64-bit)
Cache: yes
Test command: setup.py test
master hash: dcb2475
branch hash: c1bd939

Automatic review by SymPy Bot.

rlamy added some commits Jun 5, 2011
@rlamy rlamy Simplify code in sympy.core.logic
* Remove 'op' class attribute
* Allow And() and Or()
* Use a set in AndOr_Base.__new__
* Remove debugging code and superfluous comments
ea2cca9
@rlamy rlamy Represent negated facts as Not('fact') instead of '!fact'.
* Remove sympy.core.logic.name_not().
846364f
@rlamy rlamy Create class FactKB to hold old-style assumption dict
Since deduce_all_facts() mutates the assumption dict, it makes more
sense as a method of FactKB.
9bed0bc
@rlamy rlamy Make FactRules an attribute of FactKB.
A FactKB() should always be used with the same FactRules instance, so
it's best to specify the rules once and givethe KB a reference to it.
46a72df
@rlamy rlamy Make Logic instances picklable ebe0eb5
@rlamy rlamy Use key, val pairs for the keys of FactRules.rels 88bb7ee
@rlamy rlamy Merge rel_tbeta and rel_fbeta
+ Add helpers _base_fact() and _as_pair()
1f245d7
@rlamy rlamy Simplify some conditions inside apply_beta_to_alpha_route() 5837adb
@rlamy rlamy apply_beta_to_alpha_route(): untangle phase 1 and phase 2 d6a81f7
@rlamy rlamy Remove AssumeMixin._learn_new_facts()
The only role of this method was to ensure that instances have their own
copy of the assumptions before writing into them, but since assumptions
are written to whenever _what_known_about() is called, the copy can be
performed before calling it, without any loss of efficiency.
a4890b6
@rlamy rlamy Simplify Function initialisation
* Don't let Symbol.__call__ set meaningless options
* Don't allow meaningless options
f106e17
@rlamy rlamy Merge rel_tt and rel_ft into rel_t 5d73409
@rlamy rlamy Merge rel_t and rel_f. af27f10
@rlamy rlamy Split insanely long test functions in test_spin.py
They are now only outrageously long!
27f3830
@rlamy rlamy Fix typo in test_spin.py
The first JzKetCoupled in the modified test had a '1/2' instead
of 'S(1)/2'.
764ecde
@rlamy rlamy Fix ordering of KroneckerDelta arguments 4a105f0
@rlamy rlamy Don't allow classes to set assumptions, except for Symbol.
* Add missing is_commutative or _eval_is_commutative definitions.
a58736b
@rlamy rlamy Fix handling of slices in MatrixExpr d22b6b2
@rlamy rlamy Put assumption hash into Symbol.hashable_content()
* Since _assume_type_keys is None for non-Symbols, they don't need to
deal with it.
ab0a69c
@rlamy rlamy Don't use semi-deprecated Basic.compare in _aresame 819f13e
@rlamy rlamy Add class StdFactKB having _assume_rules as its .rules class attribute
This prevents every pickle from having to include a copy
of _assume_rules.
b08d7c4
@rlamy rlamy Create class PropertyManager to handle class-level assumptions.
This separates the gathering of class assumptions from the
metaprogramming happening in metaclass WithAssumptions.
0b5a571
@rlamy rlamy _what_known_about() calls itself recursively when trying prerequisites
Calling _what_known_about() directly instead of going through self.is_foo
simplifies the code flow and removes an annoying call to
getattr(self, 'is_' + k).
f3f0d61
@rlamy rlamy Override .assumptions0 in Symbol
Since Symbol is the only case where .assumption returns a non-trivial
result, this simplifies code.
6641e40
@rlamy rlamy Transfer part of the implementation of _init_assumptions to Symbol
The two cases inside _init_assumptions are now completely separate.
For non-Symbols, self._assumptions is initialised to
cls.default_assumptions, while instances of Symbol have a fully completed
and effectively immutable assumption KB.

The attribute _assume_type_keys is now useless and is removed.
e0eb7e9
@rlamy rlamy Don't use cycle detection in _what_known_about()
Infinite recursion is now prevented by temporarily setting the value
of the "fact" being evaluated to None, so that recursive calls for
the same fact return None immediately. This value is overwritten if an
actual result is obtained.

The CycleDetected exception and the _a_in_progress attribute can thus
be removed.
2a515b4
@rlamy rlamy Move property factory from WithAssumptions metaclass to PropertyManager 0c17cb4
@rlamy rlamy Rename WithAssumptions to ManagedProperties. fba9581
@Krastanov
Member

SymPy Bot Summary: ✳️ All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYh7YVDA

Interpreter: /usr/bin/python2.6 (2.6.7-final-0)
Architecture: Linux (64-bit)
Cache: yes
Test command: setup.py test
master hash: 89f5c64
branch hash: 063d8eaa37069564e993f9767705089416e10ea2

Automatic review by SymPy Bot.

@rlamy rlamy referenced this pull request Apr 21, 2012
Closed

Misc1 #1257

@rlamy
Member
rlamy commented Apr 21, 2012

Clearly, this is too big to be properly reviewed, so I've started to chop it off into smaller chunks, beginning with #1257.

@asmeurer
Member

Actually, I reviewed all of this, and only had the comments I posted above.

@rlamy
Member
rlamy commented Apr 21, 2012

Ah, OK. Well, __slots__ doesn't make anything faster directly, it only reduces memory consumption. As for benchmarks, I don't know which ones would be meaningful. In any case, I didn't notice any difference in test run time.

Was there anything else?

@asmeurer
Member

I don't think so. I still think it would be useful to store separately the assumptions that were given in addition to the computed assumptions, which should make pickling even more space efficient (because the common case is that Symbols have zero, or maybe at most one assumption set). But just the changes here are fine on their own, and already fix several problems, so that can be pushed off to a later time. So this can go in if you don't want to do that.

If it's too hard to do with the old assumptions model (I don't see why it would be, but I don't understand all the subtitles of pickling), we can hold off doing it until the new assumptions, but either way, it's something to keep in mind when looking at the design for the new assumptions.

I'll run the bot tests in Python 2.5, and use a custom test command for Python 3 that avoids the setup.py test stack overflow problem.

@asmeurer
Member

SymPy Bot Summary: 🔴 There were test failures.

@rlamy: Please fix the test failures.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYv80VDA

Interpreter: /usr/local/bin/python2.5 (2.5.0-final-0)
Architecture: Darwin (32-bit)
Cache: yes
Test command: setup.py test
master hash: 89f5c64
branch hash: 063d8eaa37069564e993f9767705089416e10ea2

Automatic review by SymPy Bot.

@asmeurer
Member

SymPy Bot Summary: ✳️ All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY770VDA

Interpreter: /Library/Frameworks/Python.framework/Versions/3.2/bin/python3 (3.2.2-final-0)
Architecture: Darwin (64-bit)
Cache: yes
Test command: ./bin/test; ./bin/doctest
master hash: 89f5c64
branch hash: 063d8eaa37069564e993f9767705089416e10ea2

Automatic review by SymPy Bot.

@asmeurer
Member

That failure is just sage stuff, so it can be ignored.

@smichr
smichr commented on 764ecde Apr 22, 2012

Should any of these uncovered lines be covered?

sympy\core\assumptions.py
    return self.__class__(self)
    BasicMeta.__init__(cls, *args, **kws)
    pmanager = PropertyManager(cls)
    cls.default_assumptions = pmanager
    if pmanager.definitions:
    raise RuntimeError("%s: class-level assumptions %s are "
    for fact in _assume_defined:
    pname = as_property(fact)
    if not hasattr(cls, pname):
    setattr(cls, pname, cls.make_property(fact))
    def getit(self):
    return self._assumptions.get(fact)
    getit.func_name = as_property(fact)
    return property(getit)
sympy\core\facts.py
    raise TautologyDetected(a,b, 'a -> a|c|...')
    raise TautologyDetected(a,b, 'a & b -> a')
    raise TautologyDetected(a,b, 'a | b -> a')
    kb, fact, value = self.args
    return "%s, %s=%s" % (kb, fact, value)
    new = self.__class__(self.rules)
    new.update(self)
    return new
sympy\core\function.py
    raise ValueError("Unknown options: %s" % options)
sympy\core\logic.py
    return self.args
sympy\core\symbol.py
    return WildFunction(self.name)(*args)
sympy\physics\secondquant.py
    obj = Basic.__new__(cls, bra, ket)
sympy\polys\polyutils.py
    except AttributeError:    # This is needed in cases like Rational :> Half
@smichr
Member
smichr commented Apr 22, 2012

regarding uncovered lines: nothing in the HasAssumptions class is covered.

@smichr
Member
smichr commented Apr 22, 2012

I sent a pull request to cover many of these lines. Note that the use of raises does not result in the line raising the error as being covered. I'm not sure if anything can be done about that. HasAssumptions should probably be checked by someone.

I'll ask for someone to check the secondquant change.

@rlamy
Member
rlamy commented Apr 22, 2012

Hmm... HasAssumptions isn't used at all. That worries me because I thought it was needed to make subsequent commits work. I need to think about it.

@smichr
Member
smichr commented Apr 22, 2012

I updated the pull request to include *args for Logic and now the line in logic is covered. But how did Logic slip past the check of test_args.py that all classes be tested for pickling?

@asmeurer
Member

test_args doesn't test that all classes are tested for pickling, just that the args are Basic (though that's not a bad idea).

rlamy and others added some commits Mar 14, 2012
@rlamy rlamy Cache the lookup of evaluation functions in PropertyManager.evaluator
This avoids a getattr call at runtime.
df3e7e1
@rlamy rlamy Streamline pickling.
* Don't rely on __slots__ for pickling, as some of the attributes are
  caches that should be regenerated.
* Implement __reduce_ex__ so that all protocols behave the same, calling
  cls.__new__() and then obj.__setstate__() when unpickling.
  In particular, this ensures that unpickling a Singleton returns the
  same object (issue 2204).
* Don't pickle _assumptions of non-Symbols, since they're not part of
  the object's definition. Fixes issue 2587.
* Fix Poly.args to return a tuple like every other class.
f2c45cb
@rlamy rlamy Move AssumeMixin._what_known_about() to PropertyManager.ask() ab34414
@rlamy rlamy Move PicklableWithSlots to polyutils
PicklableWithSlots is now only used in sympy.polys.
c9df55b
@rlamy rlamy Remove AssumeMixin.
This allows us to get rid of some tricky magic in
ManagedProperties.
9a11265
@rlamy rlamy deduce_all_facts(): Extract x_new_facts into method _tell a337653
@rlamy rlamy sympy/core/facts.py : Simplify rule processing.
Turning a -> b | c into a & !b -> c & !b is wasteful, as it'll
be split into a & !b -> c and a & !b -> !b and the latter will
be discarded.
a9258ab
@rlamy rlamy Split FactRules.rels into .full_implications and .beta_triggers
These 2 dicts are actually computed and used separately.
Not smashing them together simplifies the code and allows to
get rid of split_rules().
9b0c1b4
@rlamy rlamy Use (k, v) pairs representation in FactRules.beta_rules
This simplifies the code and guarantees that the runtime system
doesn't see any sympy.core.logic object.
a280477
@rlamy rlamy Fix docstrings in sympy/core/assumptions.py 79f5ed9
@rlamy rlamy Cleanup code and doc in FactKB(). 4826416
@smichr @rlamy smichr coverage edits
The secondquant __new__ method just calls to eval after checking args
and that returns a product or 0 (which are Basic) so I think the lines
that were deleted are just dead lines.
eb9d441
@smichr @rlamy smichr Logic takes *args; add pickling test
I'm not sure how Logic slipped past the requirement in test_args.py to
be tested.

It now takes *args and can be rebuilt from its args. Before this commit
this happened:

>>> arg = (1,);Logic(arg)
Logic(1)
>>> Logic(*_.args)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "sympy\core\logic.py", line 63, in __new__
      obj.args = tuple(args)
TypeError: 'int' object is not iterable
467d9ad
@rlamy
Member
rlamy commented Apr 25, 2012

Well, I've dropped HasAssumptions for now, I'll revisit it later. I've also added some of Chris's corrections.

@Krastanov
Member

SymPy Bot Summary: ✳️ All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY5pMWDA

Interpreter: /usr/bin/python2.6 (2.6.7-final-0)
Architecture: Linux (64-bit)
Cache: yes
Test command: setup.py test
master hash: dafbe16
branch hash: 467d9ad

Automatic review by SymPy Bot.

@asmeurer
Member

OK. Any further objections to going in?

@asmeurer
Member

SymPy Bot Summary: ✳️ All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYhrMWDA

Interpreter: /Library/Frameworks/Python.framework/Versions/3.2/bin/python3 (3.2.2-final-0)
Architecture: Darwin (64-bit)
Cache: yes
Test command: ./bin/test; ./bin/doctest
master hash: dafbe16
branch hash: 467d9ad

Automatic review by SymPy Bot.

@rlamy
Member
rlamy commented Apr 25, 2012

SymPy Bot Summary: 🔴 There were test failures.

@rlamy: Please fix the test failures.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY55MWDA

Interpreter: /usr/bin/python3 (3.2.2-final-0)
Architecture: Linux (32-bit)
Cache: yes
Test command: setup.py test
master hash: dafbe16
branch hash: 467d9ad

Automatic review by SymPy Bot.

@asmeurer
Member

That's the other issue. Python 3 tests look fine.

@smichr
Member
smichr commented Apr 26, 2012

There is now a single commit in the pull request that I made that handles the copy issue, WildFunction call issue, and filldedent issue.

@jrioux
Member
jrioux commented May 1, 2012

SymPy Bot Summary: ✳️ All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYpasWDA

Interpreter: /usr/bin/python (2.7.2-final-0)
Architecture: Linux (64-bit)
Cache: yes
Test command: setup.py test
master hash: 8426fe8
branch hash: 467d9ad

Automatic review by SymPy Bot.

@jrioux
Member
jrioux commented May 1, 2012

SymPy Bot Summary: ✳️ All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYpqsWDA

Interpreter: /usr/bin/python (2.7.0-final-0)
Architecture: Linux (32-bit)
Cache: yes
Test command: setup.py test
master hash: 8426fe8
branch hash: 467d9ad

Automatic review by SymPy Bot.

@smichr
Member
smichr commented May 2, 2012

@rlamy, I see new tests but no new commits. Have you considered the pull request (2nd) that I made to you?

@rlamy
Member
rlamy commented May 3, 2012

@smichr, I didn't change anything in my branch. I looked at your pull request, but I'd rather not use it:

  • The filldedent thing complicates something that should be very simple and guaranteed to work under all circumstances.
  • Making Wild callable is a change of behaviour. I don't think it's a good idea, and in any case this PR isn't the right place to discuss it.
  • The issues with .copy() are symptoms of a deeper design problem: it doesn't make sense for PropertyManager to subclass FactKB. But since this doesn't affect anything that's user-visible, it's rather low-priority and I'd rather deal with it later.
@smichr
Member
smichr commented May 3, 2012

I removed the filldedent commit but left the in-place definition of it to make the Inconsisten error easier to read. (I've seen it often enough to wish that it was more easily understandable.)

I didin't make Wild callable, I just removed the broken WildFunction __call__ method (which you modified and but didn't test) and added a test for Wild callability which I noticed was not tested, either. But in reviewing it I see that you did more than remove the assumptions, you changed it as

-        return Function(self.name, nargs=len(args))(*args, **self.assumptions0)
+        return Function(self.name)(*args)

But it still doesn't work with that portion you removed. In master:

>>> WildFunction('f')(x)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'WildFunction' object is not callable

So either WildFunction's call should be removed or fixed. Why was the nargs=len(args) part removed?

Regarding the .copy(); if you add a test for the uncovered lines in assumptions and facts, I'll be satisfied. That's the return self.__class__(self) line and the new = self.__class__(self.rules) and following lines; and I think you'll find that the try in PropertyManager(StdFactKB) needs to include the for-loop header.

@rlamy rlamy Make Wild() explicitly not callable
It wasn't callable previously due to WildFunction not
being callable.
d0fadc7
@Krastanov
Member

@asmeurer, @rlamy, @smichr Is this ready to go in?

It was tested on 32bit and 64bit with different versions of python by at least 3 different people, and @asmeurer already reviewed it. I would like to see this in, as I am trying to study exactly this part of sympy.

Would it be possible for the pending changes by @smichr to be submitted in a separate pull request so this can go in?

@smichr
Member
smichr commented May 5, 2012

Would it be possible for the pending changes by @smichr to be submitted in a separate pull request so this can go in?

The changes I made were in response to trying to help in getting the
changes covered with tests. In writing test, I ran into some problems.
That's why trying to get everything covered is a good idea. So rather
than waiting until you run into the problems, we can fix them today
with the changes I've proposed. And even if the current situation is
sub-optimal and will be changed, at least everything will work as
tested.

Also, Ronan has not responded to the question about why the number of
args aspect of creating a new WildFunction was removed in addition to
the assumptions. The assumptions part is ok; I don't know why the
nargs part was changed. I also don't know why the WildFunction doesn't
like to be called. That, indeed, is a different issues, but the
removal of nargs is part of the changes that were made in this pull
request and should be at least addressed before committing this, I
think. Is it because WildFunction is defined with nargs=1.

Do you want to look at the pull request that I made? I can just put
this request and mine together as a new request.

/c

@Krastanov
Member

Do you want to look at the pull request that I made? I can just put this request and mine together as a new request.

That is the problem: I hoped that this part of the code will get stable, so I can start playing with it and learning how it works without the need to study two different versions. However as you said, it seems that there are still open questions. If this is the case I should try to look at both this version and what is in master. It would be instructive, albeit being twice the work.

I will try to look at the edits. Thanks for the explanation. I may have some more questions later.

@asmeurer
Member
asmeurer commented May 5, 2012

As far as I'm concerned, this can go in. I guess we should wait for Ronan to answer Chris's question about WildFunction, and see if there are any other concerns before pushing.

@rlamy
Member
rlamy commented May 6, 2012

I added a commit to make it clear that Wild() isn't callable. BTW, the nargs keyword didn't do anything.

For the copy() coverage issue, the right fix is to put PropertyManager out of its misery: its silly name is a strong indication that this class shouldn't exist.

@smichr
Member
smichr commented May 6, 2012

I don't see the new commit yet.

Regarding the copy coverage...is there any reason not to a) include my modifications and then go ahead and discard them later or b) go ahead and add as a new commit the removal of misery?

rlamy added some commits May 7, 2012
@rlamy rlamy Store property evaluator map in the class
make_property() and _ask() don't need to refer to
PropertyManager any more.
1dae666
@rlamy rlamy Remove PropertyManager
* Merge its code with ManagedProperties
3f605a5
@rlamy
Member
rlamy commented May 7, 2012

Option b) it is, then. I still need to add a test for FactKB.copy().

@rlamy rlamy Remove FactKB.copy()
Since this method isn't used, it seems that it's actually
useless.
09a4c64
@rlamy
Member
rlamy commented May 7, 2012

Well, since the method isn't used, the easiest thing to do is just to remove it.

@smichr
Member
smichr commented May 7, 2012

Looks good. Everything passes here and only error-condition lines are uncovered:

sympy\core\facts.py
    raise TautologyDetected(a,b, 'a -> a|c|...')
    raise TautologyDetected(a,b, 'a & b -> a')
    raise TautologyDetected(a,b, 'a | b -> a')
    kb, fact, value = self.args
    return "%s, %s=%s" % (kb, fact, value)
sympy\core\function.py
    raise ValueError("Unknown options: %s" % options)
sympy\polys\polyutils.py
    except AttributeError:    # This is needed in cases like Rational :> Half

Thanks, this is in.

@smichr smichr merged commit 25904b8 into sympy:master May 7, 2012
@smichr
Member
smichr commented May 7, 2012

Could you close any related issues? I didn't see any listed but will check to see if you referenced this pull request.

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