Skip to content

Simplify er_blade#140

Closed
eric-wieser wants to merge 1 commit intopygae:masterfrom
eric-wieser:simplify-er_blade
Closed

Simplify er_blade#140
eric-wieser wants to merge 1 commit intopygae:masterfrom
eric-wieser:simplify-er_blade

Conversation

@eric-wieser
Copy link
Copy Markdown
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
Copy Markdown

codecov bot commented Dec 6, 2019

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.74%. Comparing base (d4fe3db) to head (16de7db).
⚠️ Report is 513 commits behind head on master.

Files with missing lines Patch % Lines
galgebra/ga.py 0.00% 3 Missing ⚠️

❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@utensil
Copy link
Copy Markdown
Member

utensil commented Dec 7, 2019

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

@utensil
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread galgebra/ga.py Outdated
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
@utensil utensil closed this in #552 Mar 31, 2026
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