Skip to content

Remove special-cases for scalars in contractions#99

Merged
utensil merged 1 commit intopygae:masterfrom
eric-wieser:remove-scalar-special-cases
Dec 7, 2019
Merged

Remove special-cases for scalars in contractions#99
utensil merged 1 commit intopygae:masterfrom
eric-wieser:remove-scalar-special-cases

Conversation

@eric-wieser
Copy link
Member

It's possible this will be marginally slower, but it makes everything much easier to read.

@eric-wieser eric-wieser force-pushed the remove-scalar-special-cases branch from eb69798 to 38f4c68 Compare December 2, 2019 11:43
@eric-wieser

This comment has been minimized.

@utensil
Copy link
Member

utensil commented Dec 4, 2019

So, should it actually be removed?

@eric-wieser
Copy link
Member Author

eric-wieser commented Dec 4, 2019

I need to investigate this some more. Some of the failures are just addition reordering, which can be easily fixed.

The other half look like actual bugs. Leaving this as a draft for now, I probably need a piecewise approach to work out which bit is broken.

Edit: fixed all the issues in the earlier PRs, this now works just fine

It's possible this will be marginally slower, but it makes everything much easier to read.
@eric-wieser eric-wieser changed the title Remove special-cases for when multiplicands are zero or scalars Remove special-cases for scalars in contractions Dec 6, 2019
@eric-wieser eric-wieser force-pushed the remove-scalar-special-cases branch from 4aab143 to 3e7c389 Compare December 6, 2019 15:55
@codecov
Copy link

codecov bot commented Dec 6, 2019

Codecov Report

Merging #99 into master will decrease coverage by 0.31%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #99      +/-   ##
==========================================
- Coverage   67.54%   67.23%   -0.32%     
==========================================
  Files           8        8              
  Lines        4794     4785       -9     
==========================================
- Hits         3238     3217      -21     
- Misses       1556     1568      +12
Impacted Files Coverage Δ
galgebra/ga.py 73.77% <100%> (-1.32%) ⬇️

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 159cc2a...3e7c389. Read the comment docs.

@eric-wieser
Copy link
Member Author

Some timings:

In [2]: from galgebra.ga import Ga

In [3]: g, *a = Ga.build('e*1|2|3|4')

In [4]: A, B = g.mv('A', 'mv'), g.mv('B', 'mv')

Before:

In [5]: %timeit A < B
102 ms ± 2.34 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)

After:

In [7]: %timeit A < B
105 ms ± 1.91 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)

No significant change in the general case. Probably faster for a pure scalar contraction, but that doesn't seem like a case worth optimizing for.

@eric-wieser eric-wieser marked this pull request as ready for review December 6, 2019 16:09
@eric-wieser eric-wieser requested a review from utensil December 6, 2019 16:09
Copy link
Member

@utensil utensil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can always have them back if one day these special treatments are really needed.

@utensil utensil merged commit 1e7de69 into pygae:master Dec 7, 2019
@eric-wieser eric-wieser deleted the remove-scalar-special-cases branch December 8, 2019 10:56
@utensil utensil added this to the 0.4.5 milestone Dec 18, 2019
@eric-wieser eric-wieser added the component: core Ga, Mv, Metric, etc label Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: core Ga, Mv, Metric, etc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants