Opened 3 years ago

Last modified 20 months ago

#22162 needs_review defect

Return Unknown from ex.is_xyz() if Pynac returns false

Reported by: rws Owned by:
Priority: major Milestone: sage-8.2
Component: symbolics Keywords:
Cc: pelegm Merged in:
Authors: Ralf Stephan Reviewers:
Report Upstream: N/A Work issues:
Branch: u/rws/22162-1 (Commits) Commit: 079597de7de7ff2f5474c1dd591942957818e4fc
Dependencies: Stopgaps:

Description (last modified by rws)

Expected:

sage: x.is_integer()
Unknown
sage: if x.is_real(): True
sage: if not x.is_real(): False
False

To clarify, I think it's possible to separate the issue of "why do I get False" from the issue of deciding between True/False/Unknown, by simply returning Unknown if the Pynac logic returns false.

It would be much less surprising to the user.

Later, when the complementary flags (irreal, noninteger, nonpositive) are implemented in Pynac these function can return True/Unknown/False, e.g. is_integer will return False if is_noninteger returns True, so both behave complementary.

Change History (29)

comment:1 Changed 3 years ago by rws

  • Branch set to u/rws/return_unknown_from_ex_is_xyz___if_pynac_returns_false

comment:2 Changed 3 years ago by rws

  • Authors set to Ralf Stephan
  • Commit set to 5918af7b6f0ab73bf94a340c62fab26d2992966a
  • Status changed from new to needs_review

New commits:

5918af722162: return Unknown from is_integer()

comment:3 Changed 3 years ago by rws

  • Description modified (diff)

comment:4 Changed 3 years ago by rws

  • Description modified (diff)

comment:5 follow-ups: Changed 3 years ago by jdemeyer

Regarding Unknown, shouldn't we disallow conversion to bool? Currently, bool(Unknown) is False and not Unknown is True. What do you think about that?

comment:6 Changed 3 years ago by git

  • Commit changed from 5918af7b6f0ab73bf94a340c62fab26d2992966a to 5f5d1348d22dbfe3850f542a70afbd9c447a183c

Branch pushed to git repo; I updated commit sha1. New commits:

5f5d13422162: refine doctests

comment:7 in reply to: ↑ 5 Changed 3 years ago by rws

Replying to jdemeyer:

Regarding Unknown, shouldn't we disallow conversion to bool? Currently, bool(Unknown) is False and not Unknown is True. What do you think about that?

That would have to be changed to preserve previous usage. I don't think there is much of that in Sage (except is_zero which I don't want to deal with here).

Last edited 3 years ago by rws (previous) (diff)

comment:8 in reply to: ↑ 5 ; follow-up: Changed 3 years ago by rws

  • Description modified (diff)

Replying to jdemeyer:

Regarding Unknown, shouldn't we disallow conversion to bool? Currently, bool(Unknown) is False and not Unknown is True. What do you think about that?

Actually, what I meant is that it is fine as it is, as it preserves previous usage.

comment:9 in reply to: ↑ 8 Changed 3 years ago by jdemeyer

Replying to rws:

Actually, what I meant is that it is fine as it is, as it preserves previous usage.

If the goal of this ticket is to preserve previous usage, why bother at all? Shouldn't the goal be to fix the bad previous usage instead? That means that Unknown should be treated really as an unknown value and not as a small variation on False.

comment:10 Changed 3 years ago by git

  • Commit changed from 5f5d1348d22dbfe3850f542a70afbd9c447a183c to bb6f53f68ed3cd499c60e4655ba1eb8d261b38ea

Branch pushed to git repo; I updated commit sha1. New commits:

bb6f53f22162: change more is_xyz() methods and doctests

comment:11 follow-up: Changed 3 years ago by rws

As it was a no-brainer I changed the other is_xyz() function except for comparisons with zero, and I consider the ticket complete, apart from bug or doctest fixes.

Please review.

comment:12 in reply to: ↑ 11 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_info

comment:13 follow-up: Changed 3 years ago by jdemeyer

I think we should think about 9 before merging this. What good is Unknown if it behaves the same as False for practical purposes?

comment:14 follow-up: Changed 3 years ago by rws

Well, quoting unknown.py, ..warning:: Unless PEP 335 is accepted, in the following cases, and, not and or return a somewhat wrong value::. PEP 335 was rejected so I'm being pragmatic and accept the (admittedly) small value of serving the users that don't write Sage code.

Also, isn't that an independent issue?

comment:15 in reply to: ↑ 14 ; follow-up: Changed 3 years ago by jdemeyer

Replying to rws:

Also, isn't that an independent issue?

No, I think it's not an independent issue because it's the motivation of this ticket. Which problem does this ticket solve if you don't fix Unknown?

comment:16 Changed 3 years ago by jdemeyer

There is also the purely practical point that it's easier to change the functionality of Unknown if it's only used very little.

comment:17 in reply to: ↑ 15 ; follow-up: Changed 3 years ago by rws

Replying to jdemeyer:

Which problem does this ticket solve if you don't fix Unknown?

Semantic inconsistency: Unknown is meant but the output is False.

comment:18 in reply to: ↑ 17 Changed 3 years ago by jdemeyer

Replying to rws:

Semantic inconsistency: Unknown is meant but the output is False.

With this patch, you get an object 'Unknown' which behaves a lot like False. So this doesn't fix much...

comment:19 in reply to: ↑ 13 Changed 3 years ago by rws

Replying to jdemeyer:

I think we should think about 9 before merging this. What good is Unknown if it behaves the same as False for practical purposes?

Disclaimer: Python is not my specialty.

Either you convert Unknown to bool (and you provide the means to do it via __nonzero__) or you don't (and deny any attempt). The former is this branch at the moment, the latter would force users to explicitly compare with True/False/Unknown, disallowing if is_xyz(): conditionals. It would also disallow not is_xyz() and is_xy() and/or is_yz(). By disallow I mean an exception is raised. The patch to achieve this would simply be:

diff --git a/src/sage/misc/unknown.py b/src/sage/misc/unknown.py
index b05751e..4a6979a 100644
--- a/src/sage/misc/unknown.py
+++ b/src/sage/misc/unknown.py
@@ -66,7 +66,7 @@ class UnknownClass(UniqueRepresentation, SageObject):
             sage: not Unknown
             True
         """
-        return False
+        raise NotImplementedError('Python does not support tri-valued logic.')
 
     __nonzero__ = __bool__

Unknown is used in a few places. With the patch I get about 100 doctest fails in combinat. I have not looked further into this.

comment:20 Changed 2 years ago by rws

Are the conclusions of the above comments changed if we regard Python3 richcmp? Specifically can Py3 richcmp provide an Unknown object that is more useful than simply being False, e.g., can we change the behaviour of if is_xyz(): and not is_xyz() such that the former compares with True and the latter with False?

comment:21 Changed 2 years ago by rws

Actually I find that the combinat doctest fails are fixable with a few natural changes, and so an Unknown that behaves differently from False seems possible.

comment:22 Changed 2 years ago by rws

  • Authors Ralf Stephan deleted
  • Branch u/rws/return_unknown_from_ex_is_xyz___if_pynac_returns_false deleted
  • Commit bb6f53f68ed3cd499c60e4655ba1eb8d261b38ea deleted
  • Dependencies set to #24345
  • Milestone changed from sage-7.5 to sage-8.2

See #24345.

comment:23 Changed 2 years ago by rws

  • Branch set to u/rws/22162

comment:24 Changed 2 years ago by rws

  • Authors set to Ralf Stephan
  • Commit set to c86618152fad77de88f8e07c74770adc72c20e8a
  • Dependencies #24345 deleted
  • Description modified (diff)
  • Status changed from needs_info to needs_review

New commits:

c86618122162: Return Unknown from ex.is_xyz() if Pynac returns false

comment:25 follow-up: Changed 2 years ago by rws

  • Description modified (diff)

Later, when the complementary flags (irreal, noninteger, nonpositive) are implemented in Pynac these function can return True/Unknown/False, e.g. is_integer will return False if is_noninteger returns True, so both behave complementary.

comment:26 in reply to: ↑ 25 Changed 2 years ago by vdelecroix

Replying to rws:

Later, when the complementary flags (irreal, noninteger, nonpositive) are implemented in Pynac these function can return True/Unknown/False, e.g. is_integer will return False if is_noninteger returns True, so both behave complementary.

What would be the point of having is_noninteger if is_integer is already doing the job!?

comment:27 Changed 2 years ago by rws

Agree. Exposing the flag to the user is not really necessary.

comment:28 Changed 21 months ago by rws

  • Branch changed from u/rws/22162 to u/rws/22162-1

comment:29 Changed 20 months ago by pelegm

  • Cc pelegm added
  • Commit changed from c86618152fad77de88f8e07c74770adc72c20e8a to 079597de7de7ff2f5474c1dd591942957818e4fc

New commits:

079597d22162: Return Unknown from ex.is_xyz() if Pynac returns false
Note: See TracTickets for help on using tickets.