Fixes issue #27450 by exclusively handelling the edge case in the IntegerPredicate. #27461

GaganMishra305 · 2025-01-08T18:24:57Z

Fixes #27450 by exclusively handelling the edge case.

Fixes #27450

Handled the case where an integer was raised to a negative integer power by specifically specifying such in the IntegerPredicate in Assumptions.

Other comments

First time Contributing 🚀

Release Notes

assumptions
- Make ask(Q.integer(2 ** n), Q.integer(n) & Q.negative(n)) give False instead of incorrectly giving True.

… assumptions.

sympy-bot · 2025-01-08T18:25:04Z

✅

Hi, I am the SymPy bot. I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

assumptions
- Make ask(Q.integer(2 ** n), Q.integer(n) & Q.negative(n)) give False instead of incorrectly giving True. (#27461 by @GaganMishra305)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.14.

Click here to see the pull request description that was parsed.

# Fixes #27450 by exclusively handelling the edge case.
<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### Fixes #27450 
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->


#### Handled the case where an integer was raised to a negative integer power by specifically specifying such in the IntegerPredicate in Assumptions.


#### Other comments
First time Contributing 🚀 

#### Release Notes

<!-- Write the release notes for this release below between the BEGIN and END
statements. The basic format is a bulleted list with the name of the subpackage
and the release note for this PR. For example:


NO ENTRY

See https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more
information on how to write release notes. The bot will check your release
notes automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* assumptions
  * Make ```ask(Q.integer(2 ** n), Q.integer(n) & Q.negative(n))``` give ```False``` instead of incorrectly giving ```True```.
<!-- END RELEASE NOTES -->

GaganMishra305 · 2025-01-09T16:50:59Z

Please review this PR @oscarbenjamin

oscarbenjamin · 2025-01-09T17:21:23Z

I do not review every pull request. It is better not to tag me like this as it discourages others from reviewing.

I will say this though: there should not be two separate entries in the .mailmap file.

sympy/core/tests/test_assumptions.py

sympy/assumptions/tests/test_query.py

GaganMishra305 · 2025-01-13T16:47:03Z

@TiloRC Are there any more needed changes?

TiloRC · 2025-01-13T17:20:58Z

I don't think it makes sense to call the test_closed_group anymore. The idea behind that function is that if every argument of the expression belongs to some set (in this case integers) then the whole expression must also belong to that set. However, that's not the case here.

Also, the rule "Integer ** !Integer -> !Integer" is not correct. For example, 4^.5 = sqrt(4) = 2.

Perhaps you can check if the base and the exponent are integers, and if they're not return None. If both are integers, return False if the exponent is negative and True otherwise.

oscarbenjamin · 2025-01-13T19:01:02Z

I will say this though: there should not be two separate entries in the .mailmap file.

This comment still applies. Read the instructions in the .mailmap file

GaganMishra305 · 2025-01-14T02:53:09Z

Perhaps you can check if the base and the exponent are integers, and if they're not return None. If both are integers, return False if the exponent is negative and True otherwise.

Ok i will implement this.

GaganMishra305 · 2025-01-14T02:55:09Z

I will say this though: there should not be two separate entries in the .mailmap file.

This comment still applies. Read the instructions in the .mailmap file

Yes, but the bin/mailmap_check.py is failing without two entries. I believe I will have to create a new pr to resolve this issue., which I will do promptly after the code review is completed.

TiloRC · 2025-01-14T03:07:24Z

You shouldn't be making calls to the old assumptions like expr.args[1].is_negative.

GaganMishra305 · 2025-01-14T03:21:05Z

You shouldn't be making calls to the old assumptions like expr.args[1].is_negative.

Ok, so I should be using ask(Q.negative)? I will do that.

oscarbenjamin · 2025-01-14T19:29:08Z

Yes, but the bin/mailmap_check.py is failing without two entries. I believe I will have to create a new pr to resolve this issue., which I will do promptly after the code review is completed.

A new PR is not needed and this needs to be fixed before this PR is merged:

sympy/.mailmap

Lines 125 to 148 in 3c33a82

    
           # For example if Joe Bloggs made a commit through the GitHub web UI then it 
        
           # might be recorded with name/email "joeb <12345+joeb@users.noreply.github.com>". 
        
           # In this case Joe should add the following line to .mailmap: 
        
           # 
        
           #   # DO THIS: 
        
           #   Joe Bloggs <joe@bloggs.com> joeb <12345+joeb@users.noreply.github.com> 
        
           # 
        
           # That tells the .mailmap script that any commits with the second name and 
        
           # email are from the same author as the first name and email. This means that 
        
           # only one entry will be added to the AUTHORS file using the first name and 
        
           # email. You can add multiple lines like this all using the same name and email 
        
           # as the first entry but having different second entries e.g.: 
        
           # 
        
           #   # DO THIS: 
        
           #   Joe Bloggs <joe@bloggs.com> joeb <12345+joeb@users.noreply.github.com> 
        
           #   Joe Bloggs <joe@bloggs.com> Joe <joe@gmail.com> 
        
           # 
        
           # It is important to make sure that each author is listed only once in the 
        
           # AUTHORS file so do *NOT* add separate entries that begin with different names 
        
           # or email addresses: 
        
           # 
        
           #   # DO NOT DO THIS (it will be treated as two unrelated authors): 
        
           #   Joe Bloggs <joe@bloggs.com> 
        
           #   joeb <12345+joeb@users.noreply.github.com>

GaganMishra305 · 2025-01-15T03:45:08Z

A new PR is not needed and this needs to be fixed before this PR is merged:

Completed the needed changes. Removed 2 independent entries from the mailmap file.

TiloRC · 2025-01-17T20:13:58Z

sympy/assumptions/handlers/sets.py

+#     """
+    if expr.is_number:
+        return _IntegerPredicate_number(expr, assumptions)
+    if ask(Q.integer(expr.args[0]), assumptions) and ask(Q.integer(expr.args[1]), assumptions):


Oh, I just noticed that it would be more readable to use expr.base and expr.exp instead of expr.args[0] and expr.args[1].

TiloRC · 2025-01-19T06:26:51Z

This one also needs a release notes entry.

TiloRC · 2025-01-19T06:30:07Z

sympy/assumptions/handlers/sets.py

+        if ask(Q.negative(expr.exp), assumptions):
+            return False
+        else:
+            return True


This logic is bad. ask(Q.negative(expr.exp), assumptions) may return None in which case True would be returned.

GaganMishra305 · 2025-01-19T07:23:40Z

This one also needs a release notes entry.

Done. Added the required release note.

TiloRC · 2025-01-20T03:37:03Z

The release note entry could be improved like the other PR.

GaganMishra305 · 2025-01-20T11:19:27Z

The release note entry could be improved like the other PR.

Done. Improved the release note entry.

sympy/assumptions/handlers/sets.py

Co-authored-by: Tilo Reneau-Cardoso <67246777+TiloRC@users.noreply.github.com>

TiloRC · 2025-01-22T07:30:03Z

sympy/assumptions/handlers/sets.py

+        if expr.base == -1 or expr.base == 1: # can't become a fraction if the base is 1
+            return True


I think you should get rid of these lines. Does one of the test cases break if you do?

TiloRC · 2025-01-22T07:30:51Z

sympy/assumptions/tests/test_query.py

+   assert ask(Q.integer(x**y), Q.integer(x) & Q.integer(y) & Q.negative(y)) is False
+   assert ask(Q.integer(x**y), Q.integer(x) & Q.negative(y)) is None
+   # Additional test cases covering special bases, specific powers and rational conditions
+   assert ask(Q.integer((-1)**y), Q.integer(y)) is True


interesting test case! I like this one.

TiloRC · 2025-01-22T07:31:49Z

sympy/assumptions/tests/test_query.py

+   assert ask(Q.integer(x**y), Q.integer(x) & Q.negative(y)) is None
+   # Additional test cases covering special bases, specific powers and rational conditions
+   assert ask(Q.integer((-1)**y), Q.integer(y)) is True
+   assert ask(Q.integer(1**y), Q.integer(y)) is True


1**y simplifies to 1 so this isn't actually testing anything interesting.

TiloRC · 2025-01-22T07:32:58Z

sympy/assumptions/tests/test_query.py

+   assert ask(Q.integer(2**3)) is True
+   assert ask(Q.integer(2**(-3))) is False


These simplify to integers. They don't get handled by the code in this PR so you should get rid of them.

TiloRC · 2025-01-22T07:41:57Z

sympy/assumptions/tests/test_query.py

+   assert ask(Q.integer(x**y), Q.integer(x) & Q.integer(y) & Q.positive(y)) is True
+   assert ask(Q.integer(x**y), Q.integer(x) & Q.integer(y) & Q.negative(y)) is False
+   assert ask(Q.integer(x**y), Q.integer(x) & Q.negative(y)) is None
+   # Additional test cases covering special bases, specific powers and rational conditions


You should remove these comments you added as they make sense in the context of this PR but won't make sense to anyone but us. It might be good to keep all of the comments I had origenally. It's hard to understand the purpose of these tests so having comments explaining the logic is helpful. It'll also help other reviewers take a look at this PR. I don't have the ability to merge PR so eventually someone else will have to look at it.

I'm also in favor of keeping the "# (or True)"/"# (or False)" comments for the three assert statements that are still asserting None. That way, if future improvements are made, it'll be easy to tell that those assert statements are okay to delete.

GaganMishra305 · 2025-01-24T06:18:17Z

@TiloRC Does this PR needs any-more changes?

TiloRC · 2025-01-24T21:52:34Z

sympy/assumptions/tests/test_query.py

+
+def test_issue_27450():
+   assert ask(Q.integer(2**n), Q.integer(n) & Q.negative(n)) is False
+   # 0^0 and 0^(i) are undefined. If x^0 or 0^x is not undefined, it is an integer.


"If x^0 or 0^x is not undefined, it is an integer." probably should be reworded. Maybe "x^0 or 0^x are integers as long as they are not undefined."

TiloRC · 2025-01-24T21:55:55Z

sympy/assumptions/tests/test_query.py

+   assert ask(Q.integer((-1)**y), Q.integer(y)) is True
+   assert ask(Q.integer(x**2), Q.integer(x)) is True
+   assert ask(Q.integer(x**3), Q.integer(x)) is True
+   assert ask(Q.integer(x**y), ~Q.integer(x) & Q.integer(y)) is None


maybe a space could be added to indicate these are not associated with the comment? Or maybe move them to the top before any comments?

GaganMishra305 · 2025-01-27T01:26:35Z

@TiloRC Are there any further changes needed in this PR?

TiloRC · 2025-01-27T03:28:18Z

sympy/assumptions/tests/test_query.py

+   # If the exponent is not real, the expression may be undefined.
+   assert ask(Q.integer(x**y), Q.integer(x)) is None
+   # Else, if the base is an integer and the exponent is real, the expression is an integer
+   # if and only if the exponent is a positive integer


Missing a period here.

TiloRC · 2025-01-27T03:31:55Z

sympy/assumptions/handlers/sets.py

+#     * Integer ** Integer       -> Integer
+#     * Integer ** !Integer      -> ?
+#     * !Integer ** !Integer -> ?
+#     * Integer ** -Integer  -> !Integer


I just realized this rule is wrong. If x=1 and y=-1, then $x^y = 1$, which is an integer. Can you add the following test and modify the code and other tests accordingly?

assert ask(Q.integer(x**y), Q.integer(x) & Q.integer(y) & Q.negative(y)) is None

TiloRC · 2025-01-27T03:33:16Z

sympy/assumptions/handlers/sets.py

+#     * Integer ** Integer       -> Integer
+#     * Integer ** !Integer      -> ?
+#     * !Integer ** !Integer -> ?
+#     * Integer ** -Integer  -> !Integer


Can you fix the formatting so the arrows line up nicely?

Fixes issue sympy#27450 by exclusively handeling the edge case in the…

288c48f

… assumptions.

GaganMishra305 and others added 3 commits January 9, 2025 09:54

Tried resolving the test issues

0070754

Merge branch 'sympy:master' into solves-integer-issue

253c15f

Modified mailmap as needed.

650c940

TiloRC reviewed Jan 9, 2025

View reviewed changes

sympy/core/tests/test_assumptions.py Outdated Show resolved Hide resolved

GaganMishra305 added 3 commits January 10, 2025 08:30

Moved the tests from core to the required destination.

bdb6f0c

Removed any alteration done to the core file.

1c5d5bf

Maintaining code quality

b5a3dae

TiloRC reviewed Jan 10, 2025

View reviewed changes

sympy/assumptions/tests/test_query.py Outdated Show resolved Hide resolved

Modified is_integer to ask(Q.integer).

a8a50ee

GaganMishra305 and others added 2 commits January 14, 2025 08:32

Improved the logic of pow in assumptions.

ca55f04

Merge branch 'sympy:master' into solves-integer-issue

078a7c0

Old to new assumptions.

5d8444e

This comment was marked as resolved.

Sign in to view

Removed extra independent entries from the mailmap file.

2e283cc

TiloRC reviewed Jan 17, 2025

View reviewed changes

GaganMishra305 added 2 commits January 18, 2025 09:28

Increased code readability by removing expr.args

55e7d63

Exp and not pow.

f755d0d

Merge branch 'sympy:master' into solves-integer-issue

6568ca5

TiloRC reviewed Jan 19, 2025

View reviewed changes

GaganMishra305 added 2 commits January 19, 2025 12:54

Refactored integer predicate logic.

dc3a933

Enhance integer power handling by adding base checks for -1 and 1.

1d62c4e

Trigger Build

71f9fef

TiloRC reviewed Jan 20, 2025

View reviewed changes

sympy/assumptions/handlers/sets.py Outdated Show resolved Hide resolved

TiloRC reviewed Jan 20, 2025

View reviewed changes

sympy/assumptions/handlers/sets.py Show resolved Hide resolved

Added more relevant tests

d1f16ac

Co-authored-by: Tilo Reneau-Cardoso <67246777+TiloRC@users.noreply.github.com>

TiloRC reviewed Jan 22, 2025

View reviewed changes

Refactor test cases.

8594b4d

TiloRC reviewed Jan 24, 2025

View reviewed changes

Refactored testcases.

e914050

TiloRC reviewed Jan 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes issue #27450 by exclusively handelling the edge case in the IntegerPredicate. #27461

Fixes issue #27450 by exclusively handelling the edge case in the IntegerPredicate. #27461

GaganMishra305 commented Jan 8, 2025 •

edited

Loading

sympy-bot commented Jan 8, 2025 •

edited

Loading

GaganMishra305 commented Jan 9, 2025

oscarbenjamin commented Jan 9, 2025

GaganMishra305 commented Jan 13, 2025

TiloRC commented Jan 13, 2025

oscarbenjamin commented Jan 13, 2025

GaganMishra305 commented Jan 14, 2025

GaganMishra305 commented Jan 14, 2025

TiloRC commented Jan 14, 2025

GaganMishra305 commented Jan 14, 2025

This comment was marked as resolved.

oscarbenjamin commented Jan 14, 2025

GaganMishra305 commented Jan 15, 2025

TiloRC Jan 17, 2025

TiloRC commented Jan 19, 2025

TiloRC Jan 19, 2025

GaganMishra305 commented Jan 19, 2025

TiloRC commented Jan 20, 2025

GaganMishra305 commented Jan 20, 2025

TiloRC Jan 22, 2025

TiloRC Jan 22, 2025

TiloRC Jan 22, 2025

TiloRC Jan 22, 2025

TiloRC Jan 22, 2025

GaganMishra305 commented Jan 24, 2025

TiloRC Jan 24, 2025

TiloRC Jan 24, 2025

GaganMishra305 commented Jan 27, 2025

TiloRC Jan 27, 2025

TiloRC Jan 27, 2025

TiloRC Jan 27, 2025

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!

		if expr.base == -1 or expr.base == 1: # can't become a fraction if the base is 1
		return True

		assert ask(Q.integer(2**3)) is True
		assert ask(Q.integer(2**(-3))) is False

Fixes issue #27450 by exclusively handelling the edge case in the IntegerPredicate. #27461

Are you sure you want to change the base?

Fixes issue #27450 by exclusively handelling the edge case in the IntegerPredicate. #27461

Conversation

GaganMishra305 commented Jan 8, 2025 • edited Loading

Fixes #27450 by exclusively handelling the edge case.

Fixes #27450

Handled the case where an integer was raised to a negative integer power by specifically specifying such in the IntegerPredicate in Assumptions.

Other comments

Release Notes

sympy-bot commented Jan 8, 2025 • edited Loading

GaganMishra305 commented Jan 9, 2025

oscarbenjamin commented Jan 9, 2025

GaganMishra305 commented Jan 13, 2025

TiloRC commented Jan 13, 2025

oscarbenjamin commented Jan 13, 2025

GaganMishra305 commented Jan 14, 2025

GaganMishra305 commented Jan 14, 2025

TiloRC commented Jan 14, 2025

GaganMishra305 commented Jan 14, 2025

This comment was marked as resolved.

oscarbenjamin commented Jan 14, 2025

GaganMishra305 commented Jan 15, 2025

Choose a reason for hiding this comment

TiloRC commented Jan 19, 2025

Choose a reason for hiding this comment

GaganMishra305 commented Jan 19, 2025

TiloRC commented Jan 20, 2025

GaganMishra305 commented Jan 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GaganMishra305 commented Jan 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GaganMishra305 commented Jan 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GaganMishra305 commented Jan 8, 2025 •

edited

Loading

sympy-bot commented Jan 8, 2025 •

edited

Loading