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

Handle extra symbol in diophantine #11239

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@Shekharrajak
Copy link
Member

commented Jun 14, 2016

Fixes #11236

ex:

In [ ]: eq = x**2 + 3*x*y + 4*x

In [ ]: diophantine(eq, syms=[x, z, y])
Out[ ]: set([(0, m₁, n₁), (3⋅t₀ - 4, m₁, -t₀)])

soln = diophantine(eq, param)

if len(syms) > len(var):
num_sym = numbered_symbols("m", integer=True, start=1)

This comment has been minimized.

Copy link
@asmeurer

asmeurer Jun 14, 2016

Member

Should these be dummy?

This comment has been minimized.

Copy link
@Shekharrajak

Shekharrajak Jun 14, 2016

Author Member

I followed this way. It seems Dummy should be there.

This comment has been minimized.

Copy link
@asmeurer

asmeurer Jun 14, 2016

Member

That's OK, if it's already the standard used in the module. It seems rather that the param argument to diophantine should be an iterator of symbols, rather than a single symbol. That would be an API break, though.

@asmeurer

This comment has been minimized.

Copy link
Member

commented Jun 14, 2016

I'm a little unsure about the handling of extra symbols that are returned. The fix itself looks fine, though.

@smichr any thoughts?

@Shekharrajak

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2016

Ping @asmeurer .

From current master branch:

In [2]: eq = x**2 + 3*x*y + 4*x

In [3]: diophantine(eq, syms=[x, z, y])
Out[3]: set([(0, n₁), (3⋅t₀ - 4, -t₀)])

Here user is expecting 1st value is for x, 2nd value for z and 3rd value is for y.
So in these types of cases this PR will help :

# form this PR

In [ ]: eq = x**2 + 3*x*y + 4*x

In [ ]: diophantine(eq, syms=[x, z, y])
Out[ ]: set([(0, m₁, n₁), (3⋅t₀ - 4, m₁, -t₀)])
@asmeurer

This comment has been minimized.

Copy link
Member

commented Aug 23, 2016

Would like to get a review from someone more familiar with the module (specifically about the extra symbols thing).

@Shekharrajak

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2016

@thilinarmtb

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2016

I'll go through this during the weekend.

@Shekharrajak

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2016

Thanks for reply.

@smichr

This comment has been minimized.

Copy link
Member

commented Sep 2, 2016

What about just raising an error if symbols are given that are not in the equations?

@@ -152,11 +152,24 @@ def diophantine(eq, param=symbols("t", integer=True), syms=None):
if not is_sequence(syms):
raise TypeError(
'syms should be given as a sequence, e.g. a list')
syms = [i for i in syms if i in var]

if syms != var:

This comment has been minimized.

Copy link
@smichr

smichr Sep 19, 2016

Member

Since you've made var a list, you should make syms a list, too, or else if a tuple is sent it will trivially fail this test.

This comment has been minimized.

Copy link
@Shekharrajak

Shekharrajak Oct 1, 2016

Author Member

I added the line . Thanks for review.

This comment has been minimized.

Copy link
@Shekharrajak

Shekharrajak Oct 15, 2016

Author Member

Ping @smichr , I think it is fine now.

@Shekharrajak Shekharrajak force-pushed the Shekharrajak:diophantine_extra_symbol_11236 branch from e569ebb to 9d77b62 Oct 1, 2016

@thilinarmtb

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2016

I too think we should raise an error. I am wondering about an use case where user pass an equation and then along with it pass a separate symbol list as well. Can you guys think of a scenario which forces something like that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.