SHADOWS #1508 - Don't commit #1498

Closed
wants to merge 218 commits into
from

Conversation

Projects
None yet
@smichr
Member

smichr commented Aug 18, 2012

Combinatorics #531 overhauled

I'm leaving this PR open as a shadow of #1508 which is against 0.7.2. That is the one that should be committed.

@smichr

This comment has been minimized.

Show comment
Hide comment
@smichr

smichr Aug 19, 2012

Member

I think the work on Polyhedron is ready to look at.

Member

smichr commented Aug 19, 2012

I think the work on Polyhedron is ready to look at.

+ by reversal and rotation to be give the lowest lexical ordering.
+ If no faces are given then no edges will be computed.
+
+ >>> from sympy.combinatorics.polyhedron import Polyhedron

This comment has been minimized.

@jrioux

jrioux Aug 19, 2012

Member

No need to indent.

@jrioux

jrioux Aug 19, 2012

Member

No need to indent.

This comment has been minimized.

@smichr

smichr Aug 20, 2012

Member

I'm not to the proper examples, yet, and this is more parenthetical. Is it ok to leave it indented or do you still think it should dedent.

@smichr

smichr Aug 20, 2012

Member

I'm not to the proper examples, yet, and this is more parenthetical. Is it ok to leave it indented or do you still think it should dedent.

This comment has been minimized.

@jrioux

jrioux Aug 20, 2012

Member

It's fine, I guess.

@jrioux

jrioux Aug 20, 2012

Member

It's fine, I guess.

@jrioux

View changes

sympy/combinatorics/polyhedron.py
+
+ Examples
+ ========
+ >>> from sympy.combinatorics import Polyhedron

This comment has been minimized.

@jrioux

jrioux Aug 19, 2012

Member

There needs to be an empty line before the example.

@jrioux

jrioux Aug 19, 2012

Member

There needs to be an empty line before the example.

@jrioux

This comment has been minimized.

Show comment
Hide comment
@jrioux

jrioux Aug 19, 2012

Member

Each time you have

Examples
========

it should be followed by an empty line, then the actual code example.

Member

jrioux commented Aug 19, 2012

Each time you have

Examples
========

it should be followed by an empty line, then the actual code example.

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 20, 2012

This pull request passes (merged ed988314 into 5569bb2).

This pull request passes (merged ed988314 into 5569bb2).

@Krastanov

This comment has been minimized.

Show comment
Hide comment
@Krastanov

Krastanov Aug 20, 2012

Member

SymPy Bot Summary: ✳️ All tests have passed.

Test command: setup.py test
master hash: 5569bb2
branch hash: ed988314ded03a202c8498d7de0657f285e51cd7

Interpreter 1: ✳️ All tests have passed.

Interpreter: /usr/local/bin/python2.5 (2.5.6-final-0)
Architecture: Linux (64-bit)
Cache: yes

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

Interpreter 2: ✳️ All tests have passed.

Interpreter: /usr/bin/python2.7 (2.7.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

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

Interpreter 3: ✳️ All tests have passed.

Interpreter: /usr/bin/python3.2 (3.2.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

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

Build HTML Docs: ✳️ All tests have passed.

Docs build command: make html-errors
Sphinx version: 1.1.3

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

Automatic review by SymPy Bot.

Member

Krastanov commented Aug 20, 2012

SymPy Bot Summary: ✳️ All tests have passed.

Test command: setup.py test
master hash: 5569bb2
branch hash: ed988314ded03a202c8498d7de0657f285e51cd7

Interpreter 1: ✳️ All tests have passed.

Interpreter: /usr/local/bin/python2.5 (2.5.6-final-0)
Architecture: Linux (64-bit)
Cache: yes

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

Interpreter 2: ✳️ All tests have passed.

Interpreter: /usr/bin/python2.7 (2.7.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

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

Interpreter 3: ✳️ All tests have passed.

Interpreter: /usr/bin/python3.2 (3.2.3-candidate-2)
Architecture: Linux (64-bit)
Cache: yes

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

Build HTML Docs: ✳️ All tests have passed.

Docs build command: make html-errors
Sphinx version: 1.1.3

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

Automatic review by SymPy Bot.

@amakelov

This comment has been minimized.

Show comment
Hide comment
@amakelov

amakelov Aug 20, 2012

I see that some of the stuff concerning cyclic forms in permutations.py is modified here. I'm currently working on excluding singleton cycles from the cyclic form. My approach is the following: I add a _size attribute to the Permutation class, and then, when faced with something like Permutation([[2, 3], [4, 5, 6], [8]]), find the maximum index appearing in the permutation (here it's 8) and assign the size of the permutation to that + 1. Then it remains to adjust some of the other methods in the class (after I adjusted mul so that it treats permutations of different sizes as if they leave all points outside their domain fixed, all the tests passed) so that they make sense with that new approach to cyclic forms. Is that going to be harmful to this PR?

I see that some of the stuff concerning cyclic forms in permutations.py is modified here. I'm currently working on excluding singleton cycles from the cyclic form. My approach is the following: I add a _size attribute to the Permutation class, and then, when faced with something like Permutation([[2, 3], [4, 5, 6], [8]]), find the maximum index appearing in the permutation (here it's 8) and assign the size of the permutation to that + 1. Then it remains to adjust some of the other methods in the class (after I adjusted mul so that it treats permutations of different sizes as if they leave all points outside their domain fixed, all the tests passed) so that they make sense with that new approach to cyclic forms. Is that going to be harmful to this PR?

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 20, 2012

This pull request fails (merged c46c2bb5 into 5569bb2).

This pull request fails (merged c46c2bb5 into 5569bb2).

@smichr

This comment has been minimized.

Show comment
Hide comment
@smichr

smichr Aug 20, 2012

Member

I see that some of the stuff concerning cyclic forms in permutations.py is modified here. I'm currently working on excluding singleton cycles from the cyclic form. My approach is the following: I add a _size attribute to the Permutation class, and then, when faced with something like Permutation([[2, 3], [4, 5, 6], [8]]), find the maximum index appearing in the permutation (here it's 8) and assign the size of the permutation to that + 1.

I thought of this, too, but the problem is that you don't know if they left out higher singletons (e.g. should [9] be in the permutation?)

Then it remains to adjust some of the other methods in the class (after I adjusted mul so that it treats permutations of different sizes as if they leave all points outside their domain fixed, all the tests passed) so that they make sense with that new approach to cyclic forms. Is that going to be harmful to this PR?

If you want to exclude them in the returned value of cyclic_form you can use the method reduced_cyclic_form.

If you have a 1-based cyclic_form you can use the function cyclic(cycle_form, n) to fill it out with singletons. I think I will modify it so you can optionally not subtract 1 from each element.

I wouldn't have been able to even talk about this 2 days ago, but I did a lot to combinaatorics over the past few days, so if you want to discuss this more it would be helpful.

Member

smichr commented Aug 20, 2012

I see that some of the stuff concerning cyclic forms in permutations.py is modified here. I'm currently working on excluding singleton cycles from the cyclic form. My approach is the following: I add a _size attribute to the Permutation class, and then, when faced with something like Permutation([[2, 3], [4, 5, 6], [8]]), find the maximum index appearing in the permutation (here it's 8) and assign the size of the permutation to that + 1.

I thought of this, too, but the problem is that you don't know if they left out higher singletons (e.g. should [9] be in the permutation?)

Then it remains to adjust some of the other methods in the class (after I adjusted mul so that it treats permutations of different sizes as if they leave all points outside their domain fixed, all the tests passed) so that they make sense with that new approach to cyclic forms. Is that going to be harmful to this PR?

If you want to exclude them in the returned value of cyclic_form you can use the method reduced_cyclic_form.

If you have a 1-based cyclic_form you can use the function cyclic(cycle_form, n) to fill it out with singletons. I think I will modify it so you can optionally not subtract 1 from each element.

I wouldn't have been able to even talk about this 2 days ago, but I did a lot to combinaatorics over the past few days, so if you want to discuss this more it would be helpful.

@smichr

This comment has been minimized.

Show comment
Hide comment
@smichr

smichr Aug 20, 2012

Member

OK, so now you can convert a 0-based cyclic form into a 1-based cyclic form with or without singletons by using the one_based() function.

1-based -> 0-based-full with cyclic
0-based-full -> reduced-0-based-full with reduced_cyclic_form
0-based -> 1-based (full or reduced) with one_based.

Member

smichr commented Aug 20, 2012

OK, so now you can convert a 0-based cyclic form into a 1-based cyclic form with or without singletons by using the one_based() function.

1-based -> 0-based-full with cyclic
0-based-full -> reduced-0-based-full with reduced_cyclic_form
0-based -> 1-based (full or reduced) with one_based.

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 20, 2012

This pull request passes (merged 0c94e217 into 5569bb2).

This pull request passes (merged 0c94e217 into 5569bb2).

@amakelov

This comment has been minimized.

Show comment
Hide comment
@amakelov

amakelov Aug 20, 2012

@smichr : the problem with higher singletons can be fixed in one of two ways:

  • as in my previous comment. We'll just have to make the functions treating two possibly different permutation allow different sizes. Additionally, we can supply an optional argument to the constructor that specifies the size of the permutation, something like a = Permutation([[2, 3, 4]], 20)
  • more ambitious: make a new class, ExtendedArrayForm or something, with a field _array_form that holds the usual array form of a permutation. Then we overload the __getitem__ method so that if the index is outside the bounds of self._array_form we return the index unchanged. Of course, we'll have to overload other things, like the __len__ and __str__ to make it behave like a list. Then instead of using a list to initialize the array form of a permutation, we use the corresponding ExtendedArrayForm. This will make all permutations behave as if they are acting on a practically infinite domain, and if we do it that way, we won't have to make any changes to the methods in Permutation - everything is going to work as expected, no casework like if len(a) > len(b),... will be needed. So this sounds like a rather elegant approach. On the other hand, I'm not entirely sure if it is possible to make it completely like a list, and also it doesn't seem like a very performance-efficient decision since ExtendedArrayForm instances will be created all the time.

And by the way, are we going to switch to 1-based form only? It's going to be hard and make the code uglier than it is right now; also, is it a good idea to have two standards for representing permutations -- isn't that going to get confusing?

@smichr : the problem with higher singletons can be fixed in one of two ways:

  • as in my previous comment. We'll just have to make the functions treating two possibly different permutation allow different sizes. Additionally, we can supply an optional argument to the constructor that specifies the size of the permutation, something like a = Permutation([[2, 3, 4]], 20)
  • more ambitious: make a new class, ExtendedArrayForm or something, with a field _array_form that holds the usual array form of a permutation. Then we overload the __getitem__ method so that if the index is outside the bounds of self._array_form we return the index unchanged. Of course, we'll have to overload other things, like the __len__ and __str__ to make it behave like a list. Then instead of using a list to initialize the array form of a permutation, we use the corresponding ExtendedArrayForm. This will make all permutations behave as if they are acting on a practically infinite domain, and if we do it that way, we won't have to make any changes to the methods in Permutation - everything is going to work as expected, no casework like if len(a) > len(b),... will be needed. So this sounds like a rather elegant approach. On the other hand, I'm not entirely sure if it is possible to make it completely like a list, and also it doesn't seem like a very performance-efficient decision since ExtendedArrayForm instances will be created all the time.

And by the way, are we going to switch to 1-based form only? It's going to be hard and make the code uglier than it is right now; also, is it a good idea to have two standards for representing permutations -- isn't that going to get confusing?

@jrioux

This comment has been minimized.

Show comment
Hide comment
@jrioux

jrioux Aug 20, 2012

Member

While building docs:
Warning, treated as error:
doc/src/modules/combinatorics/permutations.rst:11: WARNING: autodoc can't import/find method 'sympy.combinatorics.permutations.perm_af_parity', it reported error: "perm_af_parity", please check your spelling and sys.path

Member

jrioux commented Aug 20, 2012

While building docs:
Warning, treated as error:
doc/src/modules/combinatorics/permutations.rst:11: WARNING: autodoc can't import/find method 'sympy.combinatorics.permutations.perm_af_parity', it reported error: "perm_af_parity", please check your spelling and sys.path

@smichr

This comment has been minimized.

Show comment
Hide comment
@smichr

smichr Aug 20, 2012

Member

@jrioux , I got the sphinx working here and can check my own work now. The wrapping of the first See Also in partitions looks bad, however, as it appears to be trying to fill the paragraph instead of using ragged-right filling. And I added the LatticeOps change here to see that it would work in tests but will rebase this out once your request is in.

Member

smichr commented Aug 20, 2012

@jrioux , I got the sphinx working here and can check my own work now. The wrapping of the first See Also in partitions looks bad, however, as it appears to be trying to fill the paragraph instead of using ragged-right filling. And I added the LatticeOps change here to see that it would work in tests but will rebase this out once your request is in.

@smichr smichr reopened this Aug 23, 2012

@smichr

This comment has been minimized.

Show comment
Hide comment
@smichr

smichr Aug 23, 2012

Member

I did a total overhaul on #531. There are some other issues that were addressed along the way (e.g. quick_sort for LatticeOps args and dealing with evalf(1)) since these were causing test failures. I have already reviewed @saptman 's work but now need someone to look over the changes that I've made.

Member

smichr commented Aug 23, 2012

I did a total overhaul on #531. There are some other issues that were addressed along the way (e.g. quick_sort for LatticeOps args and dealing with evalf(1)) since these were causing test failures. I have already reviewed @saptman 's work but now need someone to look over the changes that I've made.

@smichr

This comment has been minimized.

Show comment
Hide comment
@smichr

smichr Aug 23, 2012

Member

@certik , you asked @saptman about whether this could be finalized...I worked with him quite a bit in review and discussion and so felt that I could do the final touches. It ended up being a lot more than that! (And I learned a lot in the process.) Perhaps you would have a chance to look this over?

Member

smichr commented Aug 23, 2012

@certik , you asked @saptman about whether this could be finalized...I worked with him quite a bit in review and discussion and so felt that I could do the final touches. It ended up being a lot more than that! (And I learned a lot in the process.) Perhaps you would have a chance to look this over?

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 23, 2012

This pull request passes (merged f73cd8a1 into 333ca3a).

This pull request passes (merged f73cd8a1 into 333ca3a).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 24, 2012

This pull request passes (merged b86464d8 into a184841).

This pull request passes (merged b86464d8 into a184841).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 24, 2012

This pull request passes (merged 4b001a71 into 333ca3a).

This pull request passes (merged 4b001a71 into 333ca3a).

@smichr

This comment has been minimized.

Show comment
Hide comment
@smichr

smichr Aug 24, 2012

Member

I'll close this here in favor of getting it in with 0.7.2 (#1508)

Member

smichr commented Aug 24, 2012

I'll close this here in favor of getting it in with 0.7.2 (#1508)

@smichr smichr closed this Aug 24, 2012

smichr and others added some commits Sep 13, 2012

perm_groups: minor edits of insert
There is no need to append element [0,...,cmin] to the end of the
range starting from cmin since negative indices handle this just
fine.

ig was changed to gi since it represents g[i] and ig is similar to jg
which is short for JerrumGraph.

There is no need to do double work in testing to see if g is Identity
and then search for the min change. If there is no min change then
it's the identity. The only caveat would be if one got a g like
[2,1,0,3,4,5] and alpha was 3: None would be returned and the routine
would have failed. For safety, an assertion could be made that if i
is None then g[:cmin] == range(cmin)...this has not been done and no
test has failed to it was left out.
Merge pull request #15 from pernici/combinatorics
Changed is_subgroup to admit that a group can be subgroup of a group with larger degree
fix trouble with hash and PermutationGroup
The redefining of __eq__ required that __hash__ also be defined
as noted now in the __eq__ docstring that was edited in Basic.
Merge pull request #16 from pernici/combinatorics
Added default argument `strict` to is_transitive`; fixed bug in `base`
@smichr

This comment has been minimized.

Show comment
Hide comment
@smichr

smichr Sep 18, 2012

Member

#1508 is now in.

Member

smichr commented Sep 18, 2012

#1508 is now in.

@smichr smichr closed this Sep 18, 2012

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 7, 2013

Coverage Status

Changes Unknown when pulling 419d310 on smichr:combinatorics into * on sympy:master*.

Coverage Status

Changes Unknown when pulling 419d310 on smichr:combinatorics into * on sympy:master*.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 15, 2013

Coverage Status

Changes Unknown when pulling 419d310 on smichr:combinatorics into * on sympy:master*.

Coverage Status

Changes Unknown when pulling 419d310 on smichr:combinatorics into * on sympy:master*.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 19, 2013

Coverage Status

Changes Unknown when pulling 419d310 on smichr:combinatorics into * on sympy:master*.

Coverage Status

Changes Unknown when pulling 419d310 on smichr:combinatorics into * on sympy:master*.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 20, 2013

Coverage Status

Changes Unknown when pulling 419d310 on smichr:combinatorics into * on sympy:master*.

Coverage Status

Changes Unknown when pulling 419d310 on smichr:combinatorics into * on sympy:master*.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 21, 2013

Coverage Status

Changes Unknown when pulling 419d310 on smichr:combinatorics into * on sympy:master*.

Coverage Status

Changes Unknown when pulling 419d310 on smichr:combinatorics into * on sympy:master*.

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