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

Fix examples #17

Merged
merged 65 commits into from Jul 7, 2019

Conversation

@utensil
Copy link
Member

utensil commented Mar 1, 2019

In GAlgebra 0.4.3, the first version claims to support Python 3, most of the examples are actually not working under both Python 2.7 and 3. All the examples needs to be convert to a format that's compatible with both Python 2.7 and 3, add them to CI to verify them, and fix bugs (it turns out that I've introduced quite a few of them when converting GAlgebra to support Python 3) along the way.

Here's the work needed to be done:

  • Fix examples/Terminal and verify them in CI
  • Fix examples/LaTeX and verify them in CI
  • Fix #27
  • Fix examples/Old Format and verify them in CI
  • Gather all examples that run in CI to examples/ipython
  • Fix #15
  • Fix #18
  • Fix #26
  • Add Circle CI
  • Fix for Python 3.4-3.7
  • Fix #29
  • Fix #30
  • Change the name to better describe the PR
  • Fix #32
  • Implement a part of #25
  • Add other Python 3 versions to CI (by using CircleCI)
  • Add CodeCov to Circle CI and remove TravisCI build for PRs
  • Add doc describing freezing dep to SymPy 1.3, see #31
  • Write README.md for test and examples

It's also part of #3 .

DISCLAIMER:

This PR only make sure the example can be executed and give exactly the same output under Python 2.7 and 3 (that is actually 3.6, the only Python 3 version that CI runs against but when all these work are finished, I'll add all the Python 3 version that is supported by sympy, namely 3.4, 3.5, 3.6 and 3.7).

This PR does not include the efforts to make sure all the math is sound, that's a lot of manual work, and is not practical due to my time constraint. It can be done after this PR is merged.

@utensil utensil requested review from arsenovic and hugohadfield Mar 1, 2019
@utensil utensil self-assigned this Mar 1, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 1, 2019

Codecov Report

Merging #17 into master will increase coverage by 17.37%.
The diff coverage is 88.32%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #17       +/-   ##
===========================================
+ Coverage   49.14%   66.51%   +17.37%     
===========================================
  Files           7        8        +1     
  Lines        4922     4898       -24     
===========================================
+ Hits         2419     3258      +839     
+ Misses       2503     1640      -863
Impacted Files Coverage Δ
galgebra/lt.py 43.09% <100%> (+20.87%) ⬆️
galgebra/ga.py 74.32% <100%> (+9.51%) ⬆️
galgebra/utils.py 66.66% <100%> (+4.16%) ⬆️
galgebra/mv.py 61.61% <28.57%> (+11.61%) ⬆️
galgebra/printer.py 77.34% <75.86%> (+36.11%) ⬆️
galgebra/metric.py 70.53% <89.28%> (+13.08%) ⬆️
galgebra/deprecated.py 98.46% <98.46%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5e46d2...567001a. Read the comment docs.

@utensil

This comment has been minimized.

Copy link
Member Author

utensil commented Mar 1, 2019

@FreddyBaudine Please check out https://nbviewer.jupyter.org/github/pygae/galgebra/blob/15-print-pow/examples/ipython/st4.ipynb to see whether the LaTeX output is expected and the math is sound.

@FreddyBaudine

This comment has been minimized.

Copy link

FreddyBaudine commented Mar 1, 2019

Dear Utensil,

The LateX output is OK albeit a bit different from Alan Bromborsky’s, in that the quantities to be squared are not parenthesised first. But at any rate, no confusion is possible between powers and superscripts.

As for the maths, as far as I can tell, they are OK.

Speaking of mathematics, a couple of years back, I had an email exchange with Alan Bromborsky about his intention to develop the ‘Split Differential Operator’ in view of implementing a general ‘Dot’ notation for differential operators, as described in his Galgebra: a Geometric Algebra for Sympy. This device is used a great deal in the book after Chris Doran and Anthony Lasenby, Geometric Algebra for Physicists. In those days it had not yet been implemented.
He said that he was used to writing the doc first, then program. I trust his duties must have prevented him from completing his great work.
Is there a chance of having it implemented in a future release?

Many thanks for your help.

Kind regards,
Freddy Baudine

@utensil

This comment has been minimized.

Copy link
Member Author

utensil commented Mar 2, 2019

The LateX output is OK albeit a bit different from Alan Bromborsky’s, in that the quantities to be squared are not parenthesised first. But at any rate, no confusion is possible between powers and superscripts.

You mean the LaTex source by "parenthesised"( i.e. braces ) , right? Are the math outputs identical too?

LaTeX doesn't accept consecutive superscripts, so no braces is not an valid syntax. And for the semantics, the superscripts are merely indices and part of the base of the power, so it makes sense for the function name and superscripts to be in a brace for the ^ to understand them as a whole base.

So I don't see a way around this without the verbosity of braces although I think there could be a smart way to figure out whether there are superscripts or subscripts for the function so the LaTex source could be more succinct for plain functions. But the recursive nature of printing implementation make it hard to predict all the scenarios of _printing a function so it's simpler and safer to wrap them in braces in both cases for now.

@utensil

This comment has been minimized.

Copy link
Member Author

utensil commented Mar 2, 2019

Speaking of mathematics, a couple of years back, I had an email exchange with Alan Bromborsky about his intention to develop the ‘Split Differential Operator’ in view of implementing a general ‘Dot’ notation for differential operators, as described in his Galgebra: a Geometric Algebra for Sympy. This device is used a great deal in the book after Chris Doran and Anthony Lasenby, Geometric Algebra for Physicists. In those days it had not yet been implemented.
He said that he was used to writing the doc first, then program. I trust his duties must have prevented him from completing his great work.
Is there a chance of having it implemented in a future release?

I've open an issue #19 for this and let's discuss it further there.

@FreddyBaudine

This comment has been minimized.

Copy link

FreddyBaudine commented Mar 2, 2019

Dear Utensil,

I owe you an apology for I am afraid I ill-expressed myself. In my comment above, when I compared Alan Bromborsky’s output and yours, I had in mind what is actually printed out in the PDF files not the LateX side.
To wit, the following example.
Bromborsky’ output
image
Your output
image

In your version, the actual powers are placed a little higher than the indices, hence no confusion is possible.

At any rate, the fact that (in the PDF file) components of multivectors that have to be squared are not enclosed in parentheses makes for more compact formulae.

Kind regards,
Freddy Baudine

@utensil

This comment has been minimized.

Copy link
Member Author

utensil commented Mar 2, 2019

Oh I see, although it's more compact without the parenthesis but it's much more clear with the parenthesis.

Let me see how this relates to the original implementation......

Sent with GitHawk

@utensil

This comment has been minimized.

Copy link
Member Author

utensil commented Mar 4, 2019

In https://github.com/sympy/sympy/blob/sympy-0.7.6.1/sympy/printing/latex.py#L173 there seems to be a mechanism to determine whether to add parenthesis.

and there's even a parenthesize in https://github.com/sympy/sympy/blob/sympy-1.1/sympy/printing/latex.py#L485 .

Also, the two argument version of _print does exist in the base class Printer but it seems to have encountered some problems calling methods in the super class.

@utensil

This comment has been minimized.

Copy link
Member Author

utensil commented Mar 4, 2019

I would like to further examine the best way to fix this. The current fix could be considered as a usable one but there's plenty room to improve. So I'm revoking the review and have changed the status back to WIP.

Also I would like to fix #18 and possibly #19 (comment) as well.

@utensil

This comment has been minimized.

Copy link
Member Author

utensil commented Mar 4, 2019

It seems _print_Pow has a pretty good test coverage and easy to make it 100%: https://codecov.io/gh/pygae/galgebra/src/15-print-pow/galgebra/printer.py#L644

@utensil

This comment has been minimized.

Copy link
Member Author

utensil commented Mar 8, 2019

Interesting, galgebra is not coloring the bases correctly in terminal on Travis, investigating......

image

And under 2.7, it printed parentheses which is easy to fix:

image

EDIT:

The former is caused by different defaults for Windows and Unix:

image

@utensil utensil force-pushed the 15-print-pow branch 6 times, most recently from 78845b0 to 8688903 Mar 11, 2019
@utensil utensil force-pushed the 15-print-pow branch from e98c190 to 4948bb9 Apr 12, 2019
1. the max parallelism is 4
2. �mainly support Python 2.7, 3.6, 3.7, with 3.4 for verifying the lowest Python 3 version supported
@utensil utensil force-pushed the 15-print-pow branch from 00591f3 to 978973b Apr 22, 2019
@utensil utensil force-pushed the 15-print-pow branch from 978973b to 0c3089d Apr 22, 2019
utensil added 3 commits Apr 22, 2019
@utensil utensil force-pushed the 15-print-pow branch from d2d921a to 60464b2 May 2, 2019
utensil added 4 commits May 2, 2019
by 08f33f9
between 2.7 and 3:

1. e_t v.s. eₜ
2. e_θ v.s. eₜₕₑₜₐ
1. specify base names by only prefix
2. document that only sympy<=1.3 is supported for now
3. correct instructions for testing galgebra
@utensil utensil force-pushed the 15-print-pow branch from 6e25fcf to b7db762 May 14, 2019
utensil added 2 commits May 14, 2019
@utensil

This comment has been minimized.

Copy link
Member Author

utensil commented Jun 3, 2019

@hugohadfield @arsenovic This PR is finally finished and ready for review.

Sorry during the process of fixing the examples, I end up opening many new issues and fixing them, and this PR has become very big and takes 3 months to finish. I can't exactly break them into smaller PRs since the commits are now strongly connected. All of these make it very difficult to review.

So here's my suggestion for how to review:

  • the main thread listed all the tasks of this PR, 19 of them, please first review these tasks
  • #17 (comment) listed some of the tasks left for future PRs
  • I've properly documented the changes in this PR in multiple README.md files, please search README.md in the diff, namely (ordered by importance)
    • README.md
    • test/README.md
    • examples/README.md
    • doc/books/Macdonald/README.md
    • examples/Macdonald/README.md
    • doc/README.md
@utensil utensil requested review from arsenovic and hugohadfield Jun 3, 2019
@utensil

This comment has been minimized.

Copy link
Member Author

utensil commented Jul 7, 2019

This is ready to merge and I'll make a new release based on it.

(remainder of comment moved to #86 (comment) by @eric-wieser)

@utensil utensil merged commit d3df02b into master Jul 7, 2019
7 checks passed
7 checks passed
ci/circleci: python-2.7 Your tests passed on CircleCI!
Details
ci/circleci: python-3.4 Your tests passed on CircleCI!
Details
ci/circleci: python-3.6 Your tests passed on CircleCI!
Details
ci/circleci: python-3.7 Your tests passed on CircleCI!
Details
codebeat 67 issues resolved and 50 introduced
Details
codecov/patch 88.32% of diff hit (target 49.14%)
Details
codecov/project 66.51% (+17.37%) compared to b5e46d2
Details
@utensil utensil mentioned this pull request Jul 7, 2019
4 of 9 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.