Diagram Drawing #1430
Diagram Drawing #1430
Conversation
|
SymPy Bot Summary: 🔴 There were test failures. @scolobb: Please fix the test failures. Test command: setup.py test Interpreter 1: ✳️ All tests have passed. Interpreter: /usr/local/bin/python2.5 (2.5.6-final-0) Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYl-MgDA Interpreter 2: 🔴 There were test failures. Interpreter: /usr/bin/python2.7 (2.7.3-candidate-2) Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY2aEhDA Interpreter 3: 🔴 There were test failures. Interpreter: /usr/bin/python3.2 (3.2.3-candidate-2) Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYr9sgDA Build HTML Docs: 🔴 There were test failures. Docs build command: make html-errors Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY3cghDA Automatic review by SymPy Bot. |
|
SymPy Bot Summary: 🔴 There were test failures. @scolobb: Please fix the test failures. Test command: setup.py test Interpreter 1: 🔴 There were test failures. Interpreter: /usr/local/bin/python2.5 (2.5.6-final-0) Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYn-MgDA Interpreter 2: 🔴 There were test failures. Interpreter: /usr/bin/python2.7 (2.7.3-candidate-2) Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY7PIgDA Interpreter 3: 🔴 There were test failures. Interpreter: /usr/bin/python3.2 (3.2.3-candidate-2) Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY_8AhDA Build HTML Docs: 🔴 There were test failures. Docs build command: make html-errors Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYxqkhDA Automatic review by SymPy Bot. |
|
SymPy Bot Summary: 🔴 There were test failures. @scolobb: Please fix the test failures. Test command: setup.py test Interpreter 1: 🔴 There were test failures. Interpreter: /usr/local/bin/python2.5 (2.5.6-final-0) Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYt_8hDA Interpreter 2: 🔴 There were test failures. Interpreter: /usr/bin/python2.7 (2.7.3-candidate-2) Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYvaYiDA Interpreter 3: 🔴 There were test failures. Interpreter: /usr/bin/python3.2 (3.2.3-candidate-2) Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY7pYiDA Build HTML Docs: 🔴 There were test failures. Docs build command: make html-errors Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY1Z4iDA Automatic review by SymPy Bot. |
|
SymPy Bot Summary: 🔴 There were test failures. @scolobb: Please fix the test failures. Test command: setup.py test Interpreter 1: ✳️ All tests have passed. Interpreter: /usr/local/bin/python2.5 (2.5.6-final-0) Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYyqYiDA Interpreter 2: 🔴 There were test failures. Interpreter: /usr/bin/python2.7 (2.7.3-candidate-2) Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY-JYiDA Interpreter 3: 🔴 There were test failures. Interpreter: /usr/bin/python3.2 (3.2.3-candidate-2) Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY8O8hDA Build HTML Docs: 🔴 There were test failures. Docs build command: make html-errors Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYjo8iDA Automatic review by SymPy Bot. |
|
SymPy Bot Summary: 🔴 There were test failures. @scolobb: Please fix the test failures. Test command: setup.py test Interpreter 1: 🔴 There were test failures. Interpreter: /usr/local/bin/python2.5 (2.5.6-final-0) Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY2fQiDA Interpreter 2: 🔴 There were test failures. Interpreter: /usr/bin/python2.7 (2.7.3-candidate-2) Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY8ewiDA Interpreter 3: 🔴 There were test failures. Interpreter: /usr/bin/python3.2 (3.2.3-candidate-2) Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYh-UiDA Build HTML Docs: 🔴 There were test failures. Docs build command: make html-errors Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYnoQjDA Automatic review by SymPy Bot. |
|
SymPy Bot Summary: 🔴 There were test failures. @scolobb: Please fix the test failures. Test command: setup.py test Interpreter 1: 🔴 There were test failures. Interpreter: /usr/local/bin/python2.5 (2.5.6-final-0) Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYwdUiDA Interpreter 2: 🔴 There were test failures. Interpreter: /usr/bin/python2.7 (2.7.3-candidate-2) Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYi-UiDA Interpreter 3: ✳️ All tests have passed. Interpreter: /usr/bin/python3.2 (3.2.3-candidate-2) Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY3fQiDA Build HTML Docs: 🔴 There were test failures. Docs build command: make html-errors Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY9OwiDA Automatic review by SymPy Bot. |
|
Can you rebase this onto master? It seems the commits from diagram-layout are still shown here... |
|
@ness01 , done; sorry, I totally forgot to rebase this branch :-( |
|
On 07.08.2012 19:32, Sergiu Ivanov wrote:
No problem. I'll start the reviewing tomorrow. |
Cool! |
|
@ness01, thanks! |
|
SymPy Bot Summary: ✳️ All tests have passed. Test command: setup.py test Interpreter 1: ✳️ All tests have passed. Interpreter: /usr/local/bin/python2.5 (2.5.6-final-0) Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY9ZsjDA Interpreter 2: ✳️ All tests have passed. Interpreter: /usr/bin/python2.7 (2.7.3-candidate-2) Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY-_QiDA Interpreter 3: ✳️ All tests have passed. Interpreter: /usr/bin/python3.2 (3.2.3-candidate-2) Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYlvAhDA Build HTML Docs: ✳️ All tests have passed. Docs build command: make html-errors Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY4dUiDA Automatic review by SymPy Bot. |
|
I'm going through all commits, and after that will comment on the general structure. Once this is in (hopefully very soon) I will try the code out thoroughly and compile a report on how the diagram drawing is going. We can convert it into some bug reports (maybe) and a wiki page. Just to let you know what is coming ^^. |
|
Very good work! I have gone through all commits and have very few comments. |
| """ | ||
| Given the required information, produces the string | ||
| representation of ``morphism``. | ||
| """ |
There was a problem hiding this comment.
This function is HUGE. Do you see any way to split it up further?
There was a problem hiding this comment.
Yes, splitting it up was pretty easy. I have moved out the three branches of the huge if-statement into separate functions. The resulting functions are still relatively long: 1.5--2 screenful of text, but I believe that does not critically affect readability because of numerous chunks of repetitive code.
|
Yay. I'm through. |
Previously, some dictionaries were not written according to PEP8 whitespace requirements.
This commit fixes and expands some of the inline comments which are not very clear.
This commit fixes a couple misformulations and typos in the docstrings pertaining to diagram drawing.
This commit substitutes some straight for-loops with more expressive list comprehensions.
This commit improves the docstring of this function by explaining what it actually does instead of trying describe the code in words. Further, this commit cleans up the code by removing nested if statements and dubious conditional branches.
This method used to be very large due to the amount of heuristics it incorporated. This commit essentially factors out the three branches of the central if-statement of ``_process_morphism`` into three separate functions. The four functions resulted after this split up are all less than two screenful of text, which is easier to both read and maintain.
Similarly to the previous commit, this one splits up the longish method ``_push_labels_out`` into several methods to treat the horizontal, vertical, and diagonal cases separately and to then combine the result.
This commit is another one in the series of commits which split up longish methods in XypicDiagramDrawer by factoring out independent functionality in separate methods. This commit is particularly concerned with the method ``draw``, which now looks much tidier.
… method. This commit adds an overview comment to the beginning of ``draw`` which offers an overall perspective on the steps through which this methods passes to produce the final result.
This commit splits the testing function into four functions according to the kinds of diagrams tested.
This commit fixes the layout of section titles throughout the whole ``diagram_drawing.py`` file.
|
I have complied with the first round of comments, rebased the branch, and pushed the new version. Test coverage is 97%; I'll fix that soon. I don't know how to test a A couple weeks ago I've found some diagrams which were typeset rather uglily; I'll look into that after cranking up the test coverage. That shouldn't be considered as a stopper for merging this pull-request though; I guess it would be detrimental to keep it open for a longer time. |
|
If you can figure out how to test it, go for it. Otherwise, don't worry about it. |
|
OK, thank you. |
That sounds useful. Let's keep it in mind for later. |
|
On 11.09.2012 13:33, Sergiu Ivanov wrote:
Yes. I intend to run the drawer against many examples once it is in and |
|
On 11.09.2012 13:33, Sergiu Ivanov wrote:
I think that's fine. I'll look through the branch again as soon as I can |
Cool :-) Looking forward to your bug reports.
Very good! Thank you! |
|
I think this looks good! @asmeurer, could you run the bot again? |
|
SymPy Bot Summary: 🔴 Failed after merging scolobb/ct2-diagram-drawing (2ecadb7) into master (cf6bb17). |
|
Well that's great, isn't it? I'll push tomorrow evening if there are no complaints. |
|
Yes it is. Good work here and this summer. By the way, I found out I will be taking a category theory course next semester. So I hope that this module will be useful to me then (and also if you start seeing a bunch of bug reports on it next year, that's why). Maybe I'll even try typesetting my homework in LaTeX just so I can use the diagram drawing. |
Great! Thank you! When you have merged it, I will submit the pull request with the improved
Thank you!
Oh, sounds cool :-) By "next semester" do you refer to the semester which starts in January? I do hope that by that time I will have fixed many of the issues with ugly typesetting. I also hope I will have some commutativity proving in place, but it's hard to promise. |
|
Yes, the semester starting in January. |
|
SymPy Bot Summary: ✳️ Passed after merging scolobb/ct2-diagram-drawing (2ecadb7) into master (c49dca1). |
|
Well then, here we go! @scolobb, what's next? |
|
@ness01 , thank you! :-) My plans for the nearest future are as follows:
|
Based on the diagram layouts produced by the code in #1429, the code introduced by this pull request allows producing Xy-pic presentations of diagrams.
This pull request should only be merged after #1429.