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

Simplify er_blade #140

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Dec 6, 2019

This function did two things unnecessarily:

  • Convert blade reps to base rep before calling mul. This is already handled within mul, so there's no need to do it again at the call site.
  • Branch depending on the mode string - this branching is already handled by Mul

This function is called only by Ga.connection, which is not tested anywhere, so has no code coverage.

@codecov
Copy link

codecov bot commented Dec 6, 2019

Codecov Report

Merging #140 into master will increase coverage by 0.13%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #140      +/-   ##
==========================================
+ Coverage   68.61%   68.74%   +0.13%     
==========================================
  Files           9        9              
  Lines        4684     4675       -9     
==========================================
  Hits         3214     3214              
+ Misses       1470     1461       -9
Impacted Files Coverage Δ
galgebra/ga.py 74.15% <0%> (+0.62%) ⬆️

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 d4fe3db...16de7db. Read the comment docs.

@utensil
Copy link
Member

utensil commented Dec 7, 2019

The diff coverage is 0%. Why? Could it be not 0%?

@utensil
Copy link
Member

utensil commented Dec 7, 2019

er_blade is completely not used in any test cases. I suggest to hold changes to unused code until we have corresponding test cases to cover them or we figure out the usecase or even whether it's needed at all.

https://codecov.io/gh/pygae/galgebra/src/master/galgebra/ga.py#L1650

Codecov
Hosted coverage report highly integrated with GitHub, Bitbucket and GitLab. Awesome pull request comments to enhance your QA.

@eric-wieser
Copy link
Member Author

I suggest to hold changes to unused code until we have corresponding test cases

I'd perhaps rephrase that to witholding merging changes until we have coverage. Perhaps a tag for this type of pr would be useful.

Having said that, in this case the change takes a mysterious function to a trivial one, so might be worth merging anyway - if you follow the code path for mul, you see it converts to blade rep anyway.

@utensil utensil added the merge_postponed PRs that should not be merged until the code they affect has tests label Dec 8, 2019
@utensil
Copy link
Member

utensil commented Dec 11, 2019

witholding merging changes until we have coverage

ok.

might be worth merging anyway

Better not. Sticking to the standard, coverage should be a prerequisite.

galgebra/ga.py Outdated Show resolved Hide resolved
This function did two things unnecessarily:

* Convert blade reps to base rep before calling mul. This is already handled within `mul`, so there's no need to do it again at the call site.
* Branch depending on the mode string - this branching is already handled by `Mul`
@eric-wieser eric-wieser modified the milestone: 0.6.0 May 29, 2020
@utensil utensil added this to the 0.6.0 milestone Mar 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge_postponed PRs that should not be merged until the code they affect has tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants