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

Remove copy-pasted code in LibV2-LibV3 #314

Merged
merged 1 commit into from
Sep 14, 2023
Merged

Remove copy-pasted code in LibV2-LibV3 #314

merged 1 commit into from
Sep 14, 2023

Conversation

anilbey
Copy link
Contributor

@anilbey anilbey commented Sep 13, 2023

Changes

I've spotted many duplicated codes in LibV3. This PR removes them, which will help us get a true picture of our C++ coverage and make maintenance easier.

Removing duplicates means:

  • Easier updates, as changes won't need to be made in multiple places.
  • Clearer testing coverage, highlighting tested and untested functions.
  • A cleaner base if we decide to reimplement C++ features in Python later.

Note: This PR doesn't address all old code redundancies. Some functions had been previously duplicated, but subsequently only one version had been updated. We should remember to check them later.

Tag

Tagged as 5.1

Squashed commits

delete LibV2::AP_begin_indices same impl exists in LibV3

rm LibV2::AP_end_indices same impl exists in LibV3

remove LibV3 rise and fall indices, same as LibV2

remove LibV3 interpolate, same as LibV1

remove LibV3 AP_duration, same as LibV2

remove LibV3 trace check, same as LibV1

rm LibV3 peak indices, same as LibV1

rm ISI_values and ISI_CV from LibV3, same in LibV1

Remove 4 more copy pasted functions in LibV3

rm min_ahp_idx & values duplicated code from libV3

rm LibV3 LibV3::AHP_depth_abs, same as LibV1

remove 3 more copy pasted code from LibV3

remove duplicated libv3 doublet_ISI implementation

@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2023

Codecov Report

Patch coverage has no change and project coverage change: +3.63% 🎉

Comparison is base (0db6990) 49.81% compared to head (3651ebe) 53.45%.
Report is 1 commits behind head on master.

❗ Current head 3651ebe differs from pull request most recent head 9678671. Consider uploading reports for the commit 9678671 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #314      +/-   ##
==========================================
+ Coverage   49.81%   53.45%   +3.63%     
==========================================
  Files          30       30              
  Lines        8997     8390     -607     
  Branches     4079     3671     -408     
==========================================
+ Hits         4482     4485       +3     
+ Misses       1852     1263     -589     
+ Partials     2663     2642      -21     
Files Changed Coverage Δ
efel/cppcore/FillFptrTable.cpp 1.04% <ø> (+0.10%) ⬆️
efel/cppcore/LibV2.cpp 20.35% <ø> (+1.87%) ⬆️
efel/cppcore/LibV3.cpp 1.70% <ø> (+1.25%) ⬆️

... and 2 files with indirect coverage changes

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

delete LibV2::AP_begin_indices same impl exists in LibV3

rm LibV2::AP_end_indices same impl exists in LibV3

remove LibV3 rise and fall indices, same as LibV2

remove LibV3 interpolate, same as LibV1

remove LibV3 AP_duration, same as LibV2

remove LibV3 trace check, same as LibV1

rm LibV3 peak indices, same as LibV1

rm ISI_values and ISI_CV from LibV3, same in LibV1

Remove 4 more copy pasted functions in LibV3

rm min_ahp_idx & values duplicated code from libV3

rm LibV3 LibV3::AHP_depth_abs, same as LibV1

remove 3 more copy pasted code from LibV3

remove duplicated libv3 doublet_ISI implementation
Copy link
Collaborator

@AurelienJaquier AurelienJaquier left a comment

Choose a reason for hiding this comment

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

Looks good! I like the +3.6% coverage

@anilbey anilbey merged commit 740ea34 into master Sep 14, 2023
6 checks passed
@anilbey anilbey deleted the deduplicate branch September 14, 2023 07: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.

3 participants