search

openjournals / joss-reviews

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

[REVIEW]: OMG: A Scalable and Flexible Simulation and TestingEnvironment Toolbox for Intelligent Microgrid Control #2435

Closed whedon closed 3 years ago

whedon commented 4 years ago

Submitting author: @stheid (Stefan Heid) Repository: https://github.com/upb-lea/openmodelica-microgrid-gym Version: 0.2.0-2 Editor: @dhhagan Reviewer: @katyhuff, @gonsie Archive: 10.5281/zenodo.4041278

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

status

Status badge code:

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

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

@katyhuff & @gonsie, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

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

✨ Please try and complete your review in the next six weeks ✨

Review checklist for @katyhuff

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @gonsie

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @katyhuff, @gonsie it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

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

@whedon commands

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

@whedon generate pdf
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

whedon commented 4 years ago 
Reference check summary:

OK DOIs

- None

MISSING DOIs

- https://doi.org/10.5334/jors.188 may be missing for title: PyPSA: Python for Power System Analysis
- https://doi.org/10.1109/tpwrs.2018.2829021 may be missing for title: Pandapower: An Open-Source Python Tool for Convenient Modeling, Analysis, and Optimization of Electric Power Systems
- https://doi.org/10.1109/tie.2012.2194969 may be missing for title: Advanced Control Architectures for Intelligent Microgrids, Part I: Decentralized and Hierarchical Control
- https://doi.org/10.1109/tie.2012.2196889 may be missing for title: Advanced Control Architectures for Intelligent Microgrids, Part II: Power Quality, Energy Storage, and AC/DC Microgrids

INVALID DOIs

- None
dhhagan commented 3 years ago

@whedon remind @katyhuff in 2 days

whedon commented 3 years ago

Reminder set for @katyhuff in 2 days

dhhagan commented 3 years ago

@whedon remind @gonsie in 2 days

whedon commented 3 years ago

Reminder set for @gonsie in 2 days

whedon commented 3 years ago

:wave: @katyhuff, please update us on how your review is going.

whedon commented 3 years ago

:wave: @gonsie, please update us on how your review is going.

katyhuff commented 3 years ago

katyhuff Review

This is an interesting tool for testing control system approaches for microgrids. I'm genuinely interested in testing it out further and doing more exploring with it. I think the purpose of the tool is fairly clear and the audience certainly exists. It builds cleanly from source and the pip install approach works cleanly as well. While the build succeeds, the tests fail on my MacBook (pyfmi.fmi.FMUException: Error loading the binary. The FMU contains no binary for this platform.). Upon reflection, the README indicated this might be the case. So, I redid this on my ubuntu machine and things went better. The tests themselves cover the majority of functions in the code, though most are more in the style of integration tests rather than unit tests. In those cases, the expected results take the form of hdf5 files and are named somewhat vaguely (test_main2, test_main3), so the purpose of these particular tests is a bit mysterious to a new user attempting to learn from them. The API has documentation is available online and there are appropriate guides for new developers and users. I tried running some of the examples, which went ok. There are a few boxes that I'm not ready to check. I'll make specific comments regarding each of those items.

Contribution and authorship:

The submitting author (@stheid) has made major contributions to the software and the full list of paper authors seems complete. However, one entry seems to be potentially worth clarifying in the text of the paper. The contribution from Eyke Hüllermeier is not clear based on the repository history. Presumably, this individual (Eyke Hüllermeier) has contributed conceptually and intellectually. They do not seem to have committed code to the repository. This is fine, as far as I'm concerned, but should be clarified, either here in this review discussion, or potentially in the acknowledgments section of the text.

Functionality:

If the functional claims of the software were a bit more clear in the paper, then I could better confirm them. A few of the functional claims statements (particularly in the bulleted list in the paper) are a bit vague (see comments in PDF attached at the bottom of this review. Clear examples of the use of the tool are found in the codebase and will be sufficient to confirm this functionality when aspects like "many software auxiliaries" are made more precise.

Performance:

The authors briefly mention that this tool is for "fast simulation" but no quantitative measure of "fast" is provided, so I have chosen to check this box at the moment. However, I would recommend that the authors either drop the "fast" qualifier or quantify it specifically. This is just a suggestion and I'll leave it up to the authors whether to incorporate it.

Installation Instructions:

I've checked this box, but I would recommend that the readme state earlier in the instructions (or, more clearly?) that there are no FMU binaries for MacOSX, so one must build them oneself (and, in order to do so must install omc on their mac and run the fmu creation script.) This is just a suggestion and I'll leave it up to the authors whether to incorporate it.

Example Usage:

The repository contains examples which are clear and are discussed well in the readme. The paper could potentially benefit from a brief explanation of one of those examples. But, it's not 100% necessary. It just might bring the conceptual level of the paper discussion up to the level of discussing real world applications. This is just a suggestion and I'll leave it up to the authors whether to incorporate it.

Quality of writing:

There are a handful of instances in which the paper is not very clear. Actually, the README was much more informative, in part because it used clearer, more direct language. I recommend the authors consider improving their paper clarity by approaching the clear direct style of the README. In any case, I have made some scribbles on the pdf here which give suggestions regarding grammar, sentence structure, and diction which might improve the clarity of the work.

References:

The references are in great need of review and effort. The list of references may not be complete. It seems as if there are some software tools heavily used which are either not cited (PyFMI) or possibly cited improperly (Open Modelica). I have made notes in the pdf here regarding this. All references are missing DOIs. Whedon picked up on the following, but a few more DOIs are missing beyond those. Please include all of the DOIs I have noted as missing in the pdf. Note especially the missing DOIs noticed by whedon:

stheid commented 3 years ago

@katyhuff Thanks a lot for the detailed review! We are currently working on incorporating your issues and will come back to you in the upcomming weeks.

stheid commented 3 years ago

@katyhuff The master branch contains all changes proposed (https://github.com/upb-lea/openmodelica-microgrid-gym/commit/4d902722f090d8b0fb81b1b0c7ac5f0434338127).

right now the CI does not properly build the master, but this issue is already solved in develop. however, the develop is currently not in a mergable state, therefore i only cherry picked the changes to the pdf and readme from that branch

stheid commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right: Check article proof :page_facing_up: :point_left:

dhhagan commented 3 years ago

Reminder set for @gonsie in 2 days

stheid commented 3 years ago

@gonsie Is there anything we can do to continue the review progress.

@dhhagan maybe can you send out a new reminder?

dhhagan commented 3 years ago

Hi @stheid - I will send another reminder to @gonsie but otherwise will give them one more week to complete their review within the six-week period.

gonsie commented 3 years ago

gonsie review

Sorry for the delay in my review. Unfortunately, I had many technical issues: I only have access to a Mac and I seem unable to mark the checkboxes in my review section at the top of this issue 😞 That said, here is my best effort at reviewing the documentation and paper.

stheid commented 3 years ago

@gonsie Thank you for your review. We will discuss it this friday in our team and try to also find a solution for mac, but it will probably be impossible to provide lasting OSX support as our group does not have regular access to a Mac.

In general it is possible to use FMI on Mac and also the rest of our library should be crossplatform. However, we where not able to test it. What is currently platform depended are the precompiled FMUs provided by us, which are only built for Linux and Windows as we have not mac available and didn't deem the work in cross-compilation reasonable.

gonsie commented 3 years ago

Thanks @stheid. Perhaps a container image could be made available?

In any case, I’m sure that @katyhuff ‘s review covers most of the issues with actually running the software.

stheid commented 3 years ago

It would be very helpful to know if your review is based on the revised version after @katyhuff s feedback or on the orgininal version?

Questions on the feedback:

The remaining issues are clear and will be solved in the upcoming weeks. I will come back once they are merged into master.

gonsie commented 3 years ago

It would be very helpful to know if your review is based on the revised version after @katyhuff s feedback or on the orgininal version?

I don't think there is a way to view the original version? I viewed the proof on the same day that submitted my review. I think that is the updated version of the paper.

Are you referring to the page in the documentation? We don't see the issue here. Perhaps you can clarify a little more. https://upb-lea.github.io/openmodelica-microgrid-gym/parts/user_guide/examples.html

Yes I am referring to that page. The Basic examples are incomplete. The 'creating an environment page' starts with "The following is a minimal example..." but there is nothing following. No code example, or terminal commands, or image. The 'Creating an agent and runner' page has an image that does not load. The 'Creating plots of environment variables' ends with "... like shown in the example below," but there is nothing more on that page.

stheid commented 3 years ago

You are absolutely right. Something went broken with the linking to the source code, perhaps we renamed files. We will definetly check it again. Thanks a lot for discovering that !

stheid commented 3 years ago

Dear @gonsie, all changes are now merged to master.

dhhagan commented 3 years ago

@whedon remind @gonsie in 3 days

whedon commented 3 years ago

Reminder set for @gonsie in 3 days

whedon commented 3 years ago

:wave: @gonsie, please update us on how your review is going.

dhhagan commented 3 years ago

@whedon remind @gonsie in 2 days

whedon commented 3 years ago

Reminder set for @gonsie in 2 days

whedon commented 3 years ago

:wave: @gonsie, please update us on how your review is going.

gonsie commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

:point_right: Check article proof :page_facing_up: :point_left:

gonsie commented 3 years ago

@stheid Thanks for the changes. I’m satisfied with my review.

@dhhagan Thanks, I’ve completed my review.

dhhagan commented 3 years ago

Hi @katyhuff - any chance you've had time to review the proposed changes by @stheid per your review? Thanks!

katyhuff commented 3 years ago

@whedon check references

whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.3384/ecp18154206 is OK
- 10.1109/MPE.2008.918718 is OK
- 10.1016/j.energy.2017.05.123 is OK
- 10.5334/jors.188 is OK
- 10.1109/TPWRS.2018.2829021 is OK
- 10.1109/TIE.2012.2194969 is OK
- 10.1109/TIE.2012.2196889 is OK

MISSING DOIs

- None

INVALID DOIs

- None
katyhuff commented 3 years ago

@dhhagan I have had a chance to review and I'm willing to accept, though I still have one concern:

Most of my comments were either optional or were handled by the authors. My concern about functionality was primarily regarding the clarity of examples and clarity of the functional claims in the paper. I made a comment previously that the functional claims of the software should be made more clear in the paper. The authors added a brief list of keywords to clarify what was meant by "auxiliaries," which helps. But, I still think the paper could benefit from even more clarity regarding exactly what is being claimed as functionality contributed by the work.

stheid commented 3 years ago

@katyhuff Thanks for the additional clarification.

We had another discussion in the Team, but we find it difficult to add more than the bulleted list we have added. Therefore we would ask if it would be possible to successfully close the review with the given state.

katyhuff commented 3 years ago

Ok. If you feel it is as clear as it can be, then the final assessment is in the hands of @dhhagan.

dhhagan commented 3 years ago

@katyhuff Thanks for your feedback and review. @stheid: I would like to see a bit more information about the many software auxiliaries listed in the Features section. It doesn't currently tell me what is possible/available, which I think would be beneficial and help make the paper itself more clear. In addition, there are a few areas where the language could be cleaned up - I will send a PR with suggestions in a little bit and you can take/leave the ones you want and/or feel are necessary.

dhhagan commented 3 years ago

@stheid I've made a PR (https://github.com/upb-lea/openmodelica-microgrid-gym/pull/81) with some suggested text changes in addition to what was mentioned above about expanding the clarity of examples and many software auxiliaries listed above. Please let me know when you've had time to review these and I'll take another look.

stheid commented 3 years ago

Thank you very much for the PR, it is already merged :)

Additionally, we clarified this paragraph further. https://github.com/upb-lea/openmodelica-microgrid-gym/commit/de90dc9a74c1bcbe2581f7d680cfa2879ea9f6e6

dhhagan commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

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

dhhagan commented 3 years ago

@stheid Thanks for those updates. Can you please create an archive of your repo using Zenodo, FigShare or similar? Once that's done, please report the DOI of the archive back here. Let me know if you need any help with this.

stheid commented 3 years ago

It seems like Zenodo forces us to make a new release in order to get a DOI. We are currently discussing if we make a small release just for this technical reasons or if we wait for the master to be in release worthy shape.

stheid commented 3 years ago

This should be the DOI https://doi.org/10.5281/zenodo.4041278

Zenodo seems to have issues in their infrastructure right now, because the creation of this DOI took more than 4 hours. Therefore i think it should not consern that the DOI cannot be resolved yet. Hopefully it will work tomorrow.

dhhagan commented 3 years ago

@whedon check references

whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.3384/ecp18154206 is OK
- 10.1109/MPE.2008.918718 is OK
- 10.1016/j.energy.2017.05.123 is OK
- 10.5334/jors.188 is OK
- 10.1109/TPWRS.2018.2829021 is OK
- 10.1109/TIE.2012.2194969 is OK
- 10.1109/TIE.2012.2196889 is OK

MISSING DOIs

- None

INVALID DOIs

- None
dhhagan commented 3 years ago

@stheid It looks like the Zenodo archive doesn't contain the full/correct author list. Could you correct that and then post the final OMG version and DOI archive? As soon as that's updated, I can accept this manuscript. 👍