Public algorithm interface renaming using snake_case #942

albestro · 2023-07-19T13:10:55Z

Close #940

As per discussion in https://confluence.cscs.ch/display/SCISWDEV/2023-07-20+algorithm+renaming+discussion

Decisions:

B+ is the winning choice
Move public functions to top-level dlaf:: namespace
Move eigensolver building blocks to internal namespace
Leave already internal algorithm implementations in their sub-namespaces (e.g. dlaf::eigensolver::internal::Eigensolver)

TODO:

albestro · 2023-07-19T13:38:41Z

cscs-ci run

msimberg

I'm happy with the renaming. I just want to highlight that if we're renaming them there may be room for other changes:

I see backTransformation was shortened to bt. I'm fine with this. However, does back_transformation or backtransformation hurt?
gen_to_std vs generalized_to_standard
tridiag_solver vs tridiagonal_solver vs tridiagonal (the triangular solver is called dlaf::solver::triangular, the tridiagonal solver dlaf::eigensolver::tridiag_solver; why does the latter have solver twice in the name? same for dlaf::eigensolver::eigensolver vs dlaf::eigensolver::<something that doesn't repeat eigensolver>)
if we go for abbreviations why not red_to_band?

As said above, I don't want to cause further work on this if you're happy with the current names. I just want to make sure that the options were considered and:

alternatives were dismissed as worse, or
the current possible inconsistencies accepted (for a possible future further renaming)

rasolca · 2023-07-19T17:17:47Z

I'm happy with the renaming. I just want to highlight that if we're renaming them there may be room for other changes:
* I see `backTransformation` was shortened to `bt`. I'm fine with this. However, does `back_transformation` or `backtransformation` hurt?

* `gen_to_std` vs `generalized_to_standard`

* if we go for abbreviations why not `red_to_band`?

The naming was chosen to match the filename. It is not perfect as you said, but it is a good starting point.

* `tridiag_solver` vs `tridiagonal_solver` vs `tridiagonal` (the triangular solver is called `dlaf::solver::triangular`, the tridiagonal solver `dlaf::eigensolver::tridiag_solver`; why does the latter have `solver` twice in the name? same for `dlaf::eigensolver::eigensolver` vs `dlaf::eigensolver::<something that doesn't repeat eigensolver>`)

I'm not much happy with the namespace/algorithm names as well, but I didn't find any good solution yet.
What I want for the new naming is:

the algorithm name should give an idea on the operation (e.g. when using using solver::triangular; calling triangular(...) is unreadable).
the namespaces should consider dependencies, but also have a logical meaning. E.g. gen_to_std depends on everything, even if it shouldn't.
names should not be too long. the namespace and the algorithm name should not occupy a full line of code.

Any idea is welcomed to be discussed.

albestro · 2023-07-20T07:00:02Z

@msimberg @rasolca I just moved the discussion to the related issue #940 😉

albestro · 2023-07-24T09:07:29Z

cscs-ci run

albestro · 2023-07-24T10:24:45Z

Notes:

general_multiplication has not been moved to public interface yet because it needs some changes that it will go through in the next month
max_norm replaces the generic version with the argument norm(norm_type)

include/dlaf/auxiliary/norm.h

include/dlaf/eigensolver/bt_band_to_tridiag.h

msimberg · 2023-07-27T07:43:34Z

include/dlaf/eigensolver/eigensolver.h

@@ -70,13 +70,13 @@ void eigensolver(blas::Uplo uplo, Matrix<T, D>& mat, Matrix<BaseType<T>, D>& eig
 /// @param uplo specifies if upper or lower triangular part of @p mat will be referenced
 /// @param mat contains the Hermitian matrix A
 template <Backend B, Device D, class T>
-EigensolverResult<T, D> eigensolver(blas::Uplo uplo, Matrix<T, D>& mat) {
+EigensolverResult<T, D> hermitian_eigensolver(blas::Uplo uplo, Matrix<T, D>& mat) {


Just want to check, is the name EigensolverResult still accurate?

Leaving this out for now, but @rasolca we probably need your input on what you'd like to do here leter.

msimberg

No comments in addition to those made by @RMeli.

rasolca

No extra comment from my side.

RMeli · 2023-07-28T12:10:07Z

Since #886 has been merged first, the internals of the C/ScaLAPACK API will need to be updated here. @albestro, I can open a PR against your branch if you prefer, but it should be a matter of changing two calls:

DLA-Future/src/c_api/eigensolver/eigensolver.h

Lines 62 to 63 in bcc77db

    
           dlaf::eigensolver::eigensolver<dlaf::Backend::Default, dlaf::Device::Default, T>( 
        
               communicator_grid, blas::char2uplo(uplo), matrix.get(), eigenvalues.get(), eigenvectors.get());

DLA-Future/src/c_api/factorization/cholesky.h

Lines 49 to 50 in bcc77db

    
           dlaf::factorization::cholesky<dlaf::Backend::Default, dlaf::Device::Default, T>( 
        
               communicator_grid, blas::char2uplo(uplo), matrix.get());

moved to internal without renaming because it has to be changed before making it public

albestro · 2023-08-07T13:59:45Z

cscs-ci run

albestro · 2023-08-07T14:03:06Z

I'm not sure this PR fully address the original intentions of #940. We can go for merging this and later re-open it or create more specific issues (e.g. renaming of internals too)

codecov-commenter · 2023-08-07T15:41:17Z

Codecov Report

Merging #942 (08b4313) into master (bcc77db) will increase coverage by 0.06%.
The diff coverage is 97.50%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #942      +/-   ##
==========================================
+ Coverage   93.37%   93.44%   +0.06%     
==========================================
  Files         143      143              
  Lines        8654     8650       -4     
  Branches     1104     1103       -1     
==========================================
+ Hits         8081     8083       +2     
+ Misses        388      383       -5     
+ Partials      185      184       -1

Files Changed	Coverage Δ
include/dlaf/eigensolver/band_to_tridiag/api.h	`100.00% <ø> (ø)`
include/dlaf/eigensolver/gen_to_std/impl.h	`100.00% <ø> (ø)`
include/dlaf/factorization/cholesky/impl.h	`100.00% <ø> (ø)`
include/dlaf/multiplication/general.h	`85.71% <ø> (ø)`
include/dlaf/multiplication/triangular/impl.h	`100.00% <ø> (ø)`
include/dlaf/solver/triangular/impl.h	`99.62% <ø> (ø)`
include/dlaf/auxiliary/norm.h	`60.00% <62.50%> (+14.54%)`	⬆️
include/dlaf/eigensolver/band_to_tridiag.h	`54.28% <100.00%> (ø)`
include/dlaf/eigensolver/bt_band_to_tridiag.h	`100.00% <100.00%> (ø)`
include/dlaf/eigensolver/bt_reduction_to_band.h	`100.00% <100.00%> (ø)`
... and 14 more

albestro requested review from msimberg, rasolca and RMeli July 19, 2023 13:10

rasolca added the Priority:High label Jul 19, 2023

rasolca approved these changes Jul 19, 2023

View reviewed changes

RMeli approved these changes Jul 19, 2023

View reviewed changes

msimberg approved these changes Jul 19, 2023

View reviewed changes

albestro self-assigned this Jul 19, 2023

msimberg added this to the release v0.2.0 milestone Jul 19, 2023

albestro mentioned this pull request Jul 20, 2023

Refactoring snake_case #940

Closed

albestro force-pushed the alby/rename_public_algo branch from 33a6880 to 5652388 Compare July 24, 2023 09:05

albestro requested review from rasolca, msimberg and RMeli July 24, 2023 10:25

RMeli mentioned this pull request Jul 24, 2023

ScaLAPACK-like interface for Cholesky decomposition and eigensolver #886

Merged

24 tasks

RMeli approved these changes Jul 26, 2023

View reviewed changes

include/dlaf/auxiliary/norm.h Show resolved Hide resolved

include/dlaf/eigensolver/bt_band_to_tridiag.h Show resolved Hide resolved

msimberg reviewed Jul 27, 2023

View reviewed changes

msimberg approved these changes Jul 27, 2023

View reviewed changes

rasolca approved these changes Jul 28, 2023

View reviewed changes

albestro added 5 commits August 7, 2023 11:34

rename bt_band_to_tridiag

de36310

rename reduction_to_band

d932a10

rename bt_reduction_to_band

3f1b091

rename gen_to_std

b7ae094

rename gen_eigensolver

7d68e76

albestro added 16 commits August 7, 2023 11:34

rename tridiag_solver

25f2555

public: hermitian_eigensolver and EigensolverResult

3699d58

public: hermitian_generalized_eigensolver

4f8772e

public: cholesky_factorization

aa3999e

public: triangualr_multiplication

b5acb92

public: hermitian_multiplication

fbec04e

public: triangular_solver

ab28f56

internal: generalized_to_standard

278b4d5

internal: reduction_to_band

7c810ea

internal: bt_reduction_to_band

3d8d0f8

internal: band_to_tridiagonal

c82808e

internal: bt_band_to_tridiagonal

e7c53fd

internal: tridiagonal_eigensolver

08233aa

internal: general_multiplication

fcf1be6

moved to internal without renaming because it has to be changed before making it public

public: max_norm

ef216ac

fix C API

e929470

albestro force-pushed the alby/rename_public_algo branch from c8e9b41 to e929470 Compare August 7, 2023 10:46

test_norm: fix warnings

08b4313

msimberg approved these changes Aug 7, 2023

View reviewed changes

msimberg merged commit 26cf7e9 into master Aug 8, 2023
3 checks passed

msimberg deleted the alby/rename_public_algo branch August 8, 2023 07:26

github-actions bot pushed a commit that referenced this pull request Aug 8, 2023

Doc Public algorithm interface renaming using snake_case (#942)

7df1862

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Public algorithm interface renaming using snake_case #942

Public algorithm interface renaming using snake_case #942

albestro commented Jul 19, 2023 •

edited

Loading

albestro commented Jul 19, 2023

msimberg left a comment •

edited

Loading

rasolca commented Jul 19, 2023 •

edited

Loading

albestro commented Jul 20, 2023

albestro commented Jul 24, 2023

albestro commented Jul 24, 2023

msimberg Jul 27, 2023

msimberg Aug 7, 2023

msimberg left a comment

rasolca left a comment

RMeli commented Jul 28, 2023

albestro commented Aug 7, 2023

albestro commented Aug 7, 2023

codecov-commenter commented Aug 7, 2023 •

edited

Loading

Public algorithm interface renaming using snake_case #942

Public algorithm interface renaming using snake_case #942

Conversation

albestro commented Jul 19, 2023 • edited Loading

albestro commented Jul 19, 2023

msimberg left a comment • edited Loading

Choose a reason for hiding this comment

rasolca commented Jul 19, 2023 • edited Loading

albestro commented Jul 20, 2023

albestro commented Jul 24, 2023

albestro commented Jul 24, 2023

msimberg Jul 27, 2023

Choose a reason for hiding this comment

msimberg Aug 7, 2023

Choose a reason for hiding this comment

msimberg left a comment

Choose a reason for hiding this comment

rasolca left a comment

Choose a reason for hiding this comment

RMeli commented Jul 28, 2023

albestro commented Aug 7, 2023

albestro commented Aug 7, 2023

codecov-commenter commented Aug 7, 2023 • edited Loading

Codecov Report

albestro commented Jul 19, 2023 •

edited

Loading

msimberg left a comment •

edited

Loading

rasolca commented Jul 19, 2023 •

edited

Loading

codecov-commenter commented Aug 7, 2023 •

edited

Loading