Aligning the code with contributing guidelines by shakedregev · Pull Request #42 · ORNL/GridKit

shakedregev · 2025-01-16T14:59:18Z

This is my first pass at the cleanup of the code. It's not ready yet.

There's a stylistic choice of inline functions that does not really go with rest of the code. I want to understand the rationale or change it to regular functions.
Most functions do not have explanations of their parameters (and are not marked in or out)

shakedregev · 2025-01-17T21:43:06Z

I further noticed that COO_Matrix is incorrectly defined with an underscore. Is this on purpose?
I put comments in the code, you can search for "SR".

pelesh · 2025-01-18T23:08:59Z

@shakedregev, please fix build error (see CI pipeline).

shakedregev · 2025-01-22T15:08:19Z

@shakedregev, please fix build error (see CI pipeline).

Fixed

pelesh

Left a few nitpicking comments. Generally looks good. Mark when it is ready to merge.

examples/Microgrid/Microgrid.cpp

examples/RLCircuit/RLCircuit.cpp

src/LinearAlgebra/COO_Matrix.hpp

src/Model/PowerElectronics/InductionMotor/InductionMotor.cpp

src/Model/PowerElectronics/SynchronousMachine/SynchronousMachine.hpp

src/Model/PowerElectronics/SystemModelPowerElectronics.hpp

pelesh

Document COO_Matrix constructors.
Document method variables
Break lines that are too long.

src/LinearAlgebra/COO_Matrix.hpp

alexander-novo · 2025-02-13T20:36:04Z

I'm not sure this is the right place to discuss this, but since there is a lot of attention being put on COO_Matrix, I wanted to mention that I noticed that there isn't actually an include guard on the header file.

…rTransformer

changes addressed

pelesh

This is ready to merge. Great job! Only two minor changes required.

src/Model/PowerElectronics/InductionMotor/InductionMotor.cpp

examples/Grid3Bus/README.md

reid-g

Handle the previous comments by pelesh then it looks good.

* fixed some style issues and variable name selection * fixed guard for hpp file * fixed typos --------- Co-authored-by: pelesh <peless@ornl.gov>

shakedregev requested a review from reid-g January 16, 2025 14:59

shakedregev self-assigned this Jan 16, 2025

shakedregev marked this pull request as draft January 16, 2025 14:59

shakedregev requested a review from pelesh January 16, 2025 15:25

pelesh force-pushed the shaked/code_cleanup branch from 3e9da94 to b337023 Compare January 22, 2025 17:55

pelesh reviewed Jan 22, 2025

View reviewed changes

pelesh previously requested changes Jan 28, 2025

View reviewed changes

src/LinearAlgebra/COO_Matrix.hpp Outdated Show resolved Hide resolved

src/LinearAlgebra/COO_Matrix.hpp Outdated Show resolved Hide resolved

src/LinearAlgebra/COO_Matrix.hpp Outdated Show resolved Hide resolved

pelesh added documentation Improvements or additions to documentation enhancement New feature or request labels Feb 11, 2025

shakedregev marked this pull request as ready for review February 11, 2025 16:15

shakedregev requested a review from pelesh February 11, 2025 16:15

shakedregev changed the title ~~Draft: Aligning the code with contributing guidelines~~ Aligning the code with contributing guidelines Feb 11, 2025

pelesh reviewed Feb 11, 2025

View reviewed changes

src/LinearAlgebra/COO_Matrix.hpp Show resolved Hide resolved

pelesh reviewed Feb 11, 2025

View reviewed changes

src/LinearAlgebra/COO_Matrix.hpp Outdated Show resolved Hide resolved

pelesh reviewed Feb 11, 2025

View reviewed changes

src/LinearAlgebra/COO_Matrix.hpp Outdated Show resolved Hide resolved

shakedregev added 11 commits February 14, 2025 15:07

fixed some style issues and variable name selection

cbb3294

further code cleanup and commenting

8aed1cb

installed enzyme and ran checks

82f3752

addressing comments

f81acaa

addressing comments

ff90b92

fixed sparse matrix

ccaa6c4

addressing comments

5ab9bbc

addressing comments

5d7fc96

fixed index type

b259843

fixed sorting inconsistency

c9bd81c

updated synchronous machine names

171cd64

shakedregev added 3 commits February 14, 2025 15:07

updated synchronous machine names

49fee8f

fixed notation and updated documentation for InductionMotor and Linea…

bf69b70

…rTransformer

fixed guard for hpp file

5ade72f

shakedregev force-pushed the shaked/code_cleanup branch from abe37bc to 5ade72f Compare February 14, 2025 20:10

straggling typo

01b87ab

shakedregev requested a review from pelesh February 14, 2025 20:14

pelesh requested changes Feb 17, 2025

View reviewed changes

src/Model/PowerElectronics/InductionMotor/InductionMotor.cpp Outdated Show resolved Hide resolved

examples/Grid3Bus/README.md Outdated Show resolved Hide resolved

reid-g approved these changes Feb 17, 2025

View reviewed changes

pelesh added 3 commits February 17, 2025 17:41

[skip ci] Update comment in InductionMotor.cpp

3e9b2dd

[skip ci]Update equation in Grid3Bus/README.md

7a0081f

[skip ci] Fix latex error.

2af2f9e

pelesh self-requested a review February 17, 2025 22:52

pelesh approved these changes Feb 17, 2025

View reviewed changes

pelesh merged commit bc3f969 into develop Feb 17, 2025

pelesh deleted the shaked/code_cleanup branch March 14, 2025 03:36

pelesh added a commit that referenced this pull request Apr 14, 2025

Aligning the code with contributing guidelines (#42)

d616929

* fixed some style issues and variable name selection * fixed guard for hpp file * fixed typos --------- Co-authored-by: pelesh <peless@ornl.gov>

Conversation

shakedregev commented Jan 16, 2025

Uh oh!

shakedregev commented Jan 17, 2025

Uh oh!

pelesh commented Jan 18, 2025

Uh oh!

shakedregev commented Jan 22, 2025

Uh oh!

pelesh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pelesh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexander-novo commented Feb 13, 2025

Uh oh!

pelesh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

reid-g left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants