Add polish to Bundle Types #225
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
add convert for tangent bundles add tests and fix
oxinabox
commented
Oct 11, 2023
| if lastindex(tangent.partials) == exp2(b.i) - 1 | ||
| return tangent.partials[end] | ||
| end | ||
| # TODO: should we also allow other indexes if all the partials at that level are equal up regardless of order? |
There was a problem hiding this comment.
the comment wasn't in there before.
But I don't actually see why we don't do this.
We just need to check all things at the indexes j that have count_ones(j) = b.i are equal. And if so return that thing.
if not give error about ambig
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #225 +/- ##
==========================================
+ Coverage 55.76% 56.28% +0.51%
==========================================
Files 28 28
Lines 2819 2841 +22
==========================================
+ Hits 1572 1599 +27
+ Misses 1247 1242 -5
☔ View full report in Codecov by Sentry. |
oscardssmith
approved these changes
Oct 12, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR does several things that make basically no user facing change.
It was the follow up to #224 but actually ended up way more involved.
ExplictTangentBundle[CanonicalTangentIndex(i)](which is funny that it was missing as that's the natural way to index anExplictTangentBundlegetindexmain definitions down on to to the tangent types.Uniform->Taylor/ExplictandTaylor->Explict, though in future we could put the other way around in (but they are not always going to be defined)hashfor everything, this just uses thehashof the primal for bundles, which I think is actually sufficient. I am making the assumption that we do not end up needing a dict which stores the same primal with different tangents a ton of times. We can revisit that later if it comes up.ProductTangent, as we were not using it, and I didn't want to implement the above for it. (I thinkProductTangentwas basically the same asCompositeTangentbut less complete, and that's gone now anyway)