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

Add ch7, covariant tests #381

Merged
merged 9 commits into from Nov 4, 2021
Merged

Add ch7, covariant tests #381

merged 9 commits into from Nov 4, 2021

Conversation

sritchie
Copy link
Member

@sritchie sritchie commented Oct 31, 2021

This PR addresses issues noticed by @phasetr in ch7 of FDG. I also noticed a number of tests in covariant_derivative.scm that I had missed; porting those exposed a bug in the text, which is fixed as of this PR.

From the CHANGELOG:

  • sicmutils.calculus.coordinate/generate moves to
    sicmutils.calculus.manifold/c:generate; this supports a bugfix where
    1-dimensional manifolds like R1-rect, aka the-real-line, return a
    coordinate prototype of a single element like t instead of a structure
    with a single entry, like (up t). Thanks to @phasetr for the bug report
    that led to this fix, and @gjs for finding and fixing the bug.

  • same.ish/Approximate implemented for sicmutils.structure/Structure,
    allowing ish? comparison of up and down structures with approximate
    entries. Require sicmutils.generator for this feature. (NOTE: because
    protocols are implemented for the LEFT argument, (ish? <vector> (down ...)) will still return true if the values are approximately equal, even
    though a <vector> is technically an up and should NOT equal a down. Do
    an explicit conversion to up using sicmutils.structure/vector->up if
    this distinction is important.)

  • same.ish/Approximate now defers to sicmutils.value/= for equality
    between Symbol and other types. This lets ish? handle equality between
    symbols like 'x and literal expressions that happen to wrap a single
    symbol.

  • Cartan->Cartan-over-map now does NOT compose (differential map) with its
    internal Cartan forms. This fixed a bug in a code listing in section 7.3 of
    FDG.

  • Section 7.3 of FDG implemented as tests in sicmutils.fdg.ch7-test.

  • Many new tests and explorations ported over from covariant-derivative.scm.
    These live in sicmutils.calculus.covariant-test.

  • timeout exceptions resulting from full GCD are now caught in tests using
    sicmutils.simplify/hermetic-simplify-fixture. Previously, setting a low
    timeout where simplification failed would catch and move on in normal work,
    but fail in tests where fixtures were applied.

@sritchie
Copy link
Member Author

@phasetr, I will write more later, but I spoke with Sussman and you found a legit bug in the code from the book!

The fix is in the definition of g. Basically ((differential gamma) d:dt) should just be d:dt, since (differential gamma) is already applied to the d:dt argument inside ofCartan->Cartan-over-map:

https://github.com/sicmutils/sicmutils/blob/master/src/sicmutils/calculus/covariant.cljc#L228

Same fix for the scheme code in the book. Here is the correct g, and the beginnings of some tests that lock this in:

https://github.com/sicmutils/sicmutils/pull/381/files#diff-6731d08ad0eb5e693e47f0bebee3b1095fee2b86b99ff1606500aea548b53bbeR99-R103

@sritchie sritchie changed the title Add ch7, curvature tests [in progress] Add ch7, covariant tests [in progress] Nov 1, 2021
@sritchie
Copy link
Member Author

sritchie commented Nov 1, 2021

@phasetr ACTUALLY after another round with Sussman, and now that I understand the problem better, this PR will fix the library so that the code from the book works as is. That conceptually makes more sense. Hold tight until I get more tests ported and get this merged!

@codecov-commenter
Copy link

codecov-commenter commented Nov 1, 2021

Codecov Report

Merging #381 (9da4158) into master (f406f69) will increase coverage by 0.53%.
The diff coverage is 88.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #381      +/-   ##
==========================================
+ Coverage   84.07%   84.61%   +0.53%     
==========================================
  Files          97       97              
  Lines       12876    12886      +10     
  Branches      688      691       +3     
==========================================
+ Hits        10826    10903      +77     
+ Misses       1362     1292      -70     
- Partials      688      691       +3     
Impacted Files Coverage Δ
src/sicmutils/calculus/coordinate.cljc 86.04% <ø> (+0.94%) ⬆️
src/sicmutils/calculus/manifold.cljc 76.74% <77.77%> (+0.93%) ⬆️
src/sicmutils/calculus/basis.cljc 84.84% <100.00%> (ø)
src/sicmutils/calculus/covariant.cljc 85.46% <100.00%> (+16.53%) ⬆️
src/sicmutils/simplify.cljc 97.72% <100.00%> (+2.34%) ⬆️
src/sicmutils/numsymb.cljc 93.20% <0.00%> (+0.48%) ⬆️
src/sicmutils/rational_function.cljc 79.88% <0.00%> (+0.56%) ⬆️
src/sicmutils/calculus/indexed.cljc 79.14% <0.00%> (+0.94%) ⬆️
src/sicmutils/numerical/ode.cljc 94.38% <0.00%> (+1.12%) ⬆️
... and 4 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 f406f69...9da4158. Read the comment docs.

@sritchie sritchie changed the title Add ch7, covariant tests [in progress] Add ch7, covariant tests Nov 4, 2021
@sritchie sritchie merged commit 1381cda into master Nov 4, 2021
@sritchie sritchie deleted the sritchie/new_tests branch November 4, 2021 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants