openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
720 stars 38 forks source link

[REVIEW]: ElectricGrid.jl - A Julia-based modeling and simulation tool for power electronics-driven electric energy grids #5616

Closed editorialbot closed 1 year ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@Webbah<!--end-author-handle-- (Daniel Weber) Repository: https://github.com/upb-lea/ElectricGrid.jl Branch with paper.md (empty if default branch): main Version: v1.0.2 Editor: !--editor-->@osorensen<!--end-editor-- Reviewers: @kiranshila, @degleris1 Archive: 10.5281/zenodo.8297533

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/407a7388436da2e8ba3c93bd3d3a6b5e"><img src="https://joss.theoj.org/papers/407a7388436da2e8ba3c93bd3d3a6b5e/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/407a7388436da2e8ba3c93bd3d3a6b5e/status.svg)](https://joss.theoj.org/papers/407a7388436da2e8ba3c93bd3d3a6b5e)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@kiranshila & @degleris1, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @osorensen know.

Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest

Checklists

📝 Checklist for @kiranshila

📝 Checklist for @degleris1

editorialbot commented 1 year ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

For a list of things I can do to help you, just type:

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf
editorialbot commented 1 year ago
Software report:

github.com/AlDanial/cloc v 1.88  T=0.92 s (132.0 files/s, 751750.2 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Julia                           44           3041           1836           9210
Markdown                        21           1410              0           2888
SVG                             30              0              0           2103
Jupyter Notebook                19              0         672982           1095
YAML                             4              6              7            105
TOML                             2              3              0             85
TeX                              1              9              0             48
JavaScript                       1              0              7              1
-------------------------------------------------------------------------------
SUM:                           122           4469         674832          15535
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 1 year ago

Wordcount for paper.md is 1276

editorialbot commented 1 year ago

Failed to discover a Statement of need section in paper

editorialbot commented 1 year ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.23919/PSCC.2018.8442948 is OK
- 10.1109/TIE.2012.2194969 is OK
- 10.1016/j.energy.2017.05.123 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 1 year ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

kiranshila commented 1 year ago

Review checklist for @kiranshila

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

degleris1 commented 1 year ago

Review checklist for @degleris1

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

degleris1 commented 1 year ago

@osorensen @Webbah I have completed my review. I think some minor things should be addressed, but nothing major.

General Comments

Small Things

osorensen commented 1 year ago

Thanks a lot for completing your review so quickly, @degleris1! @Webbah, could you please report back here when the points mentioned by in the review have been addressed?

kiranshila commented 1 year ago

@osorensen and @Webbah, I've completed my checklist and review, details below.

The software seems to work as advertised and fills a need specifically in the Julia ecosystem. My remaining questions involve how the software compares with state-of-the-art. From a software perspective, my notes are mainly for future improvement as the functionality is complete. As a library consumer, however, I would probably find difficulty in using this package due to the type issues described below. There may be performance consequences, but with no point of reference I'm not sure how to compare and the authors don't make any concrete claims as such.

Review

Checklist

Software

  1. I'm not sure if Markdown is the most appropriate way to serialize the ElectricGridEnv, at least in a terminal context. You can customize this using ::MIME"text/plain" in the show method. I'd expect many people to interact with this software in the REPL, which the markdown format may be unnecessary.

  2. More custom types may benefit from a show implementation (like NodeConstructor).

  3. In several places (dict keys, modes, parameters, etc) strings are used instead of a more specific data structure. For a Julia program, this is a big code smell problem. This will hurt performance as the lack of concrete types will cause the JIT to fall back to RTTI. Additionally, this implies that an invalid state could be representable, which would require robust runtime error checking, which we want to avoid if we can.

For the case of fixed sets of parameters, structs or enums would be more appropriate. For example in the documentation section on Setting parameters, As shown above, there are 4 keys: "grid", "source", "cable" and "load"., yet the types don't prevent me from doing something like

push!(env.nc.parameters, "Foo"=>"bar")

We would always want to prefer explicit over implicit, so something like

# Concrete field from a struct, named plural as it is a vector
# env.nc.parameters.sources::Vector{Source}

@enum SourceMode swing pq droop synchronverter

struct Source
    L1::Float64
    C::Float64
    mode::SourceMode
    ...
end

would be significantly better and probably more performant. This also has a documentation benefit as I'm not sure if every parameter is documented. If I see

julia> env.nc.parameters["source"][1]
Dict{Any, Any} with 30 entries:
  "L1"           => 0.00170766
  "C"            => 3.08246e-5
  "mode"         => "Synchronverter"
  "fltr"         => "LCL"
    ...
  "X₀"           => 0

Some of these parameters I can figure out (or I have found in the documentation), but others like X₀ I'm not sure how to find. This could be field-specific knowledge that I don't know, however. But, if the set of source parameters were a struct, you can include per-field documentation so there's no ambiguity.

"""Source Parameters for ..."""
struct SourceParams
    """Inductance (nH) of ..."""
    L1
    ...
end

which could then be queried directly in the help REPL (and auto-documented) by

help?> SourceParams.L1
Inductance (nH) of ...

Lastly, for cases where there is a fixed set of string-like things we need to compare, symbols would be appropriate (and canonical in Julia). Strings are always heap-allocated, which is a perf hit.

Paper

I think the paper may be a bit on the short side. A little more background on why reinforcement learning is appropriate would be welcome (coming from the perspective of a EE with not much knowledge of RL).

Minor notes for readability, feel free to ignore:

Webbah commented 1 year ago

Thank you @kiranshila and @degleris1 for the review and the comments. I will let you know asap when the points mentioned have been addressed

osorensen commented 1 year ago

Thanks for your very good review @kiranshila!

Webbah commented 1 year ago

Thank you once again @degleris1 and @kiranshila for the very good reviews and hints!

We have adjusted the paper regarding your comments with introducing a "statement of need" section and with more content regarding RL.

Additionally we have extended the comparison to other software packages like recommended and adjusted the documentation hopefully to your satisfaction.

In fact, the choice of programming language was not trivial for us. After an extensive speed comparison of the basic software structure between Python and Julia, Julia emerged as the performance winner in terms of computation time. Since it was supposed to be an open source project, we did not consider matlab. The calculation time is very important, especially for the ad-hoc data generation that is required for RL training. Therefore we finally decided to use Julia.

We are very grateful for the suggestions and hints on our Julia implementation (dict -> struct). As this was our first Julia project, we are learning a lot and will consider the better performing ideas in the future project work.

Additionally we have updated the documentation and double checked that all parameters are documented (https://upb-lea.github.io/ElectricGrid.jl/dev/Default_Parameters/).

@osorensen, we hope that we have sufficiently addressed the points mentioned above.

Webbah commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

osorensen commented 1 year ago

Thanks a lot for the response @Webbah. @kiranshila and @degleris1, could you please look at the changes made by the authors, and see if more points on your review checklists can be completed?

degleris1 commented 1 year ago

Thanks! The statement of need addressed my main concern about the differences between your package and PowerSimulationDynamics.jl.

Webbah commented 1 year ago

@kiranshila, is there anything to edit or additional changes we can make regarding your comments that we haven't addressed yet?

kiranshila commented 1 year ago

My apologies, I’ve been out of the country a while. I will complete the review this weekend.

kiranshila commented 1 year ago

@Webbah @osorensen New revision looks excellent! All my concerns have been addressed. I've updated the checklist and recommend this for publication.

osorensen commented 1 year ago

Thanks @kiranshila!

osorensen commented 1 year ago

@Webbah, I'll read through the paper once more now and let you know if I have any editorial comments.

osorensen commented 1 year ago

@editorialbot generate pdf

osorensen commented 1 year ago

@editorialbot check references

editorialbot commented 1 year ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.23919/PSCC.2018.8442948 is OK
- 10.1109/TIE.2012.2194969 is OK
- 10.1016/j.energy.2017.05.123 is OK
- 10.17775/CSEEJPES.2018.00520 is OK

MISSING DOIs

- 10.1016/j.ifacol.2017.08.1217 may be a valid DOI for title: Reinforcement Learning for Electric Power System Decision and Control: Past Considerations and Perspectives
- 10.5334/jors.188 may be a valid DOI for title: PyPSA: Python for power system analysis
- 10.1109/tpwrs.2018.2829021 may be a valid DOI for title: pandapower—an open-source python tool for convenient modeling, analysis, and optimization of electric power systems
- 10.1109/tpwrs.2023.3303291 may be a valid DOI for title: Revisiting Power Systems Time-domain Simulation Methods and Models

INVALID DOIs

- None
osorensen commented 1 year ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.23919/PSCC.2018.8442948 is OK
- 10.1109/TIE.2012.2194969 is OK
- 10.1016/j.energy.2017.05.123 is OK
- 10.17775/CSEEJPES.2018.00520 is OK

MISSING DOIs

- 10.1016/j.ifacol.2017.08.1217 may be a valid DOI for title: Reinforcement Learning for Electric Power System Decision and Control: Past Considerations and Perspectives
- 10.5334/jors.188 may be a valid DOI for title: PyPSA: Python for power system analysis
- 10.1109/tpwrs.2018.2829021 may be a valid DOI for title: pandapower—an open-source python tool for convenient modeling, analysis, and optimization of electric power systems
- 10.1109/tpwrs.2023.3303291 may be a valid DOI for title: Revisiting Power Systems Time-domain Simulation Methods and Models

INVALID DOIs

- None

@Webbah, could you please check if the missing DOIs suggested here are correct, and if so, add them.

editorialbot commented 1 year ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

Webbah commented 1 year ago

Thanks to all of you for the review process and the great comments!

All changes have been made and the DOIs have been added

Webbah commented 1 year ago

@editorialbot generate pdf

Webbah commented 1 year ago

@editorialbot check references

editorialbot commented 1 year ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.23919/PSCC.2018.8442948 is OK
- 10.1109/TIE.2012.2194969 is OK
- 10.1016/j.energy.2017.05.123 is OK
- 10.17775/CSEEJPES.2018.00520 is OK
- 10.1016/j.ifacol.2017.08.1217 is OK
- 10.1109/TPWRS.2018.2829021 is OK

MISSING DOIs

- None

INVALID DOIs

- https://doi.org/10.5334/jors.188 is INVALID because of 'https://doi.org/' prefix
- https://doi.org/10.1109/TPWRS.2023.3303291 is INVALID because of 'https://doi.org/' prefix
Webbah commented 1 year ago

@editorialbot check references

editorialbot commented 1 year ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

editorialbot commented 1 year ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.23919/PSCC.2018.8442948 is OK
- 10.1109/TIE.2012.2194969 is OK
- 10.1016/j.energy.2017.05.123 is OK
- 10.17775/CSEEJPES.2018.00520 is OK
- 10.1016/j.ifacol.2017.08.1217 is OK
- 10.5334/jors.188 is OK
- 10.1109/TPWRS.2018.2829021 is OK
- 10.1109/TPWRS.2023.3303291 is OK

MISSING DOIs

- None

INVALID DOIs

- None
Webbah commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

osorensen commented 1 year ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Editor Tasks Prior to Acceptance

osorensen commented 1 year ago

Thanks @Webbah. Could you please go through the "Additional Author Tasks" in the issue above? Please make sure that the archive (e.g., Zenodo) has exactly the same title as this paper.

Webbah commented 1 year ago
osorensen commented 1 year ago

@editorialbot set v1.0.2 as version

editorialbot commented 1 year ago

Done! version is now v1.0.2

osorensen commented 1 year ago

@Webbah, could you please make sure the title exactly matches that of the paper?

image
osorensen commented 1 year ago

Here's an example of a recently published JOSS paper: https://zenodo.org/record/8280775

Webbah commented 1 year ago

Was just checking out and should be done now

osorensen commented 1 year ago

@editorialbot set 10.5281/zenodo.8297533 as archive

editorialbot commented 1 year ago

Done! archive is now 10.5281/zenodo.8297533

osorensen commented 1 year ago

@editorialbot recommend-accept

editorialbot commented 1 year ago
Attempting dry run of processing paper acceptance...
editorialbot commented 1 year ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.23919/PSCC.2018.8442948 is OK
- 10.1109/TIE.2012.2194969 is OK
- 10.1016/j.energy.2017.05.123 is OK
- 10.17775/CSEEJPES.2018.00520 is OK
- 10.1016/j.ifacol.2017.08.1217 is OK
- 10.5334/jors.188 is OK
- 10.1109/TPWRS.2018.2829021 is OK
- 10.1109/TPWRS.2023.3303291 is OK

MISSING DOIs

- None

INVALID DOIs

- None