Skip to content

some cleanup in the Lanczos files#726

Open
manuschneider wants to merge 3 commits intomasterfrom
Lanczos_cleanup
Open

some cleanup in the Lanczos files#726
manuschneider wants to merge 3 commits intomasterfrom
Lanczos_cleanup

Conversation

@manuschneider
Copy link
Collaborator

Avoids function names starting with _ followed by upper case

@codecov
Copy link

codecov bot commented Dec 5, 2025

Codecov Report

❌ Patch coverage is 71.42857% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 35.34%. Comparing base (e87c3a2) to head (432eaaa).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
src/linalg/Lanczos_Exp.cpp 71.42% 22 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #726      +/-   ##
==========================================
+ Coverage   32.35%   35.34%   +2.99%     
==========================================
  Files         215      215              
  Lines       36363    33014    -3349     
  Branches    14597    13136    -1461     
==========================================
- Hits        11764    11670      -94     
+ Misses      22659    19430    -3229     
+ Partials     1940     1914      -26     

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

UniTensor _invert_biCGSTAB(LinOp *Hop, const UniTensor &b, const UniTensor &Tin, const int &k,
const double &CvgCrit = 1.0e-12,
const unsigned int &Maxiter = 10000) {
static UniTensor invert_biCGSTAB_internal(LinOp *Hop, const UniTensor &b, const UniTensor &Tin,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using unnamed namespace for internal linkage is a preferred style in modern C++.

Example: https://google.github.io/styleguide/cppguide.html#Internal_Linkage

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The internal functions should have access to the namespace cytnx, though. It seems there is no good solution with namespaces for this - if I put an unnamed namespace within the cytnx namespace, then the code is not hidden but visible everywhere in cytnx. Static functions seem the better solution here.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would only be visible if the unnamed namespace was in a header that was included elsewhere.


//_Lanczos_Exp_Ut_positive(out, Hop, v0, _cvgcrit, Maxiter, verbose);
_Lanczos_Exp_Ut(out, Hop, v0, tau, _cvgcrit, Maxiter, verbose);
// Lanczos_Exp_Ut_internal_positive(out, Hop, v0, _cvgcrit, Maxiter, verbose);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove unused code?

@manuschneider manuschneider modified the milestone: v1.1.0 Dec 11, 2025
@IvanaGyro
Copy link
Collaborator

BTW, if you are modifying all function names in the file, you can just use PascalCase which is requested in the coding style.

@manuschneider
Copy link
Collaborator Author

BTW, if you are modifying all function names in the file, you can just use PascalCase which is requested in the coding style.

I see that the google style sheet suggests this. However, the current API uses snake_style for all functions, and it would not make sense to change the whole API for this. To me, it makes more sense to stick to this style which is already fixed by the API, instead of mixing with google's style convention.

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