openjournals / joss-reviews

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

[REVIEW]: otoole: OSeMOSYS Tools for Energy Work #5511

Closed editorialbot closed 8 months ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@trevorb1<!--end-author-handle-- (Trevor Barnes) Repository: https://github.com/OSeMOSYS/otoole Branch with paper.md (empty if default branch): joss Version: v1.1.2 Editor: !--editor-->@fraukewiese<!--end-editor-- Reviewers: @olejandro, @fneum Archive: 10.5281/zenodo.10360538

Status

status

Status badge code:

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

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

@olejandro & @fneum, 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 @fraukewiese 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 @olejandro

📝 Checklist for @fneum

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.15 s (413.7 files/s, 79698.9 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          29           1496           1708           5461
YAML                            11             38             36           1077
reStructuredText                12            365            183            984
TeX                              1             10              0            132
INI                              1             11              0             73
Markdown                         2             25              0             55
JSON                             1              0              0             26
Bourne Shell                     2              6              4             24
make                             1              6              8             15
TOML                             1              1              3              5
-------------------------------------------------------------------------------
SUM:                            61           1958           1942           7852
-------------------------------------------------------------------------------

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

Wordcount for paper.md is 977

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

OK DOIs

- 10.1016/j.enpol.2011.06.033 is OK
- 10.12688/openreseurope.15461.1 is OK
- 10.1038/s41597-022-01737-0 is OK
- 10.1016/j.enpol.2011.09.039 is OK
- 10.1007/s12532-017-0130-5 is OK
- 10.5281/zenodo.6522795 is OK
- 10.1016/j.esr.2021.100650 is OK
- 10.1016/j.envsci.2022.07.007 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:

olejandro commented 1 year ago

Review checklist for @olejandro

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

fneum commented 1 year ago

Review checklist for @fneum

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

fneum commented 1 year ago

Excellent! This was fun reviewing.

As you can see, I already ticked all the boxes and only have a couple of questions and suggestions for you.

Paper

Repository

Docs

https://otoole.readthedocs.io/en/latest/functionality.html#otoole-results

https://otoole.readthedocs.io/en/latest/data.html#parmaters-foramt

https://otoole.readthedocs.io/en/latest/examples.html

https://otoole.readthedocs.io/en/latest/examples.html#view-results

https://otoole.readthedocs.io/en/latest/examples.html#otoole-validate

https://otoole.readthedocs.io/en/latest/examples.html#install-glpk

Functionality

trevorb1 commented 1 year ago

Thank you so much for taking the time to review and leave feedback for otoole, @fneum! We will address your notes point by point once all reviewers have completed their feedback! :)

trevorb1 commented 1 year ago

Hi @fraukewiese, I hope you are having a good summer!

I am just checking in on the review status for this paper/repository, as it has been about 2 months since the issue was created. I think one reviewer has completed their feedback, and we are just waiting on the second one.

We were hoping to do revisions and address comments once all reviews have been submitted to streamline the process. Should we continue to wait for the second reviewer to complete their feedback? Or should we begin addressing the comments from the first fully completed review?

Thanks so much!

olejandro commented 1 year ago

Hi @trevorb1, appologies for the delay on this. I will provide my feedback within about a week.

fraukewiese commented 1 year ago

@trevorb1 : You are very welcome to start adressing the comments from @fneum .

fraukewiese commented 1 year ago

@fneum : Thank you very much for your thorough review!

fraukewiese commented 1 year ago

@olejandro : Thanks for the update and your announcement to provide feedback within a week.

fraukewiese commented 1 year ago

@olejandro : How is the review going? :)

trevorb1 commented 1 year ago

Hi @fneum; thank you again for the detailed review of otoole! We really appreciate it. Please find below point-by-point responses to all of your comments. If any response is unclear, needs further explanation/discussion, or requires further attention, please let us know. Thank you!

The manuscript covers all the important points. Thank you! The manuscript has been updated to reflect all changes addressed in this review (see this commit)

The repository is in excellent condition! Thank you :)

Maybe the transformation of the raw solver output file of CPLEX could be incorporated into otoole? Agreed. otoole can now process raw output CPLEX solution files. The change has been implemented in this PR and documented here

typo "Parmaters Foramt" Thank you for noticing this! This has been corrected in this PR

Do you need both GLPK and CBC for the example or is there a way to do it with GLPK and/or CBC alone? Good point on being able to use otoole with only one solver, as this may reduce the barrier of entry for new users. Since many OSeMOSYS workflows use GLPK to build the model file, we have added functionality to process GLPK solution files. We do recognize this may be of limited use, due to the poor performance of GLPK with large models, but agree that it can reduce the learning curve for otoole.

The change has been implemented in this PR and documented here

Would it be a good idea to have an otoole clean function to tidy up after a run? This is a great idea, which we think relates closely to your otoole run idea. We have created a new issue ticket for this, however, have not implemented it.

The current scope of otoole is just the pre and post processing of data. Bridging this gap will require serious development thought on how to implement functions that remain useful to the OSeMOSYS community. Therefore, within the current scope of the project, we leave the removal of intermediate files up to the user. Please note, as elaborated in the issue ticket, in the long term it would be great to have a central configuration file for otoole to handle a wide array of options – including the automatic removal of intermediate files.

Maybe I am missing this, but I did not generate a file data.txt. Hence, I could not execute this part of the examples section. Thank you for flagging this! The data file was mistakenly not included with the documentation. This has been corrected in this PR, and the validation instructions have been updated here.

You could add [solver] installation instructions for Windows (https://winglpk.sourceforge.net/). You could add that these solvers can be installed also via conda: conda install -c conda-forge coincbc glpk (not all combinations of operating systems and solvers). Agreed. Solver download instructions have been updated to reflect Windows operating systems and users who use conda. The change has been implemented in this PR and the instructions have been updated here

Are there any use cases where otoole is used within a Python script? If yes, more targeted documentation for the front end Python API should be added besides the Module Reference. Yes, some users use otoole directly in Python scripts, rather than through the command line. We agree that documenting this process better is beneficial. A public python API has been added to otoole to allow users to convert data, read and write data, and read results without having to know the internal data structures of otoole. Moreover, dedicated examples on how to use the Python API have been added to the documentation website.

The update has been implemented in this PR and documented here

Is there an easy way to reduce the number of commands required to solve a model? E.g. combining building, solving and interpreting solver output? This is a good thought! Similar to the otoole clean discussion, we view the current scope of otoole to be only the pre and post processing data. As further discussed in this issue, creating a central configuration file where to users can launch model runs, specify solvers, and apply external scripts, is a long-term goal that requires serious development thought outside the current scope of otoole.

trevorb1 commented 1 year ago

Hi @fraukewiese. A couple quick questions on the review process:

  1. With the changes addressing @fneum's comments, we have now released otoole version 1.1.0. We originally submitted otoole under version 1.0.3. Do we have to indicate this bump in version in anyway?
  2. In the paper, we left the date as the original submission date (May 3, 2023). Does this need to be updated as we revise the paper in the review?

Thank you!

fraukewiese commented 1 year ago

@trevorb1 : In the final stage of the review process, before publishing, I will ask you to let me know the updated version number. For the paper, the idea of the original submission date is to indicate when the submission has been done initially, so it is fine to keep 3rd of May.

fraukewiese commented 1 year ago

@fneum : Are you satisfied with the response to your review by @trevorb1 ? Do you have any replies on that? And would you recommend the submission for publication?

fraukewiese commented 1 year ago

@olejandro : How is the review going? :)

fneum commented 1 year ago

I am happy with the revisions and recommend to accept this contribution.

olejandro commented 1 year ago

@fraukewiese apologies for the delay. @trevorb1 below are comments on the software paper. I'll come back shortly on the rest.

The paper is concise and generally well-written. I believe it covers most of what is required by JOSS. What is missing is a description of the state of the field. Currently it focuses solely on the OSeMOSYS users, however the problems that it is attempting to solve are common to other energy modelling communities. How does otoole, which is developed for OseMOSYS, differ from similar tools that were developed for other energy system models / model generators / frameworks?

The following sections require minor clarification / edits.

Statement of need

Extensibility

Publications

Installation and Example

trevorb1 commented 11 months ago

Hi @olejandro. Thank you so much for taking the time to review the paper! Please find below point-by-point responses to all of your comments. If any response is unclear, needs further explanation/discussion, or requires further attention, please let us know. Thank you!

Currently it focuses solely on the OSeMOSYS users, however the problems that it is attempting to solve are common to other energy modelling communities. How does otoole, which is developed for OseMOSYS, differ from similar tools that were developed for other energy system models / model generators / frameworks?

Good point! We have added a paragraph that explains the differences between how other open-source frameworks, such as PyPSA or Calliope, handle data, why it is different in OSeMOSYS’ case, and how otoole helps fill this need. Please see this commit, or specifically this paragraph, for the addition.

is a highly cited and reputable open-source framework - please provide a reference(s) to substantiate this claim (not that I disagree with it).

Good catch! We have added a reference that identifies OSeMOSYS as one of only a handful of open energy modeling frameworks mature enough to be used for energy planning studies. Moreover, we added a footnote about citations numbers via a SCOPUS search. Please see this line for the reference update and this line for the footnote.

otoole follows a strategy-based software design pattern - do you mean strategy design pattern? Please provide a reference, so there is no doubt regarding what is meant.

Yes, we mean strategy design pattern. We have updated the wording in the paper to reflect this and added a reference. Please see this line for the update.

otoole is deployed to the Python Packaging Index - I believe this should say published instead of deployed.

Agreed. The wording has been updated. Please see this line for the update.

trevorb1 commented 11 months ago

@editorialbot generate pdf

editorialbot commented 11 months ago

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

trevorb1 commented 11 months ago

@editorialbot check references

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

OK DOIs

- 10.1016/j.enpol.2011.06.033 is OK
- 10.12688/openreseurope.15461.1 is OK
- 10.1038/s41597-022-01737-0 is OK
- 10.1016/j.enpol.2011.09.039 is OK
- 10.1007/s12532-017-0130-5 is OK
- 10.5281/zenodo.6522795 is OK
- 10.1016/j.esr.2021.100650 is OK
- 10.1016/j.envsci.2022.07.007 is OK
- 10.1016/j.rser.2018.11.020 is OK
- 10.5334/jors.188 is OK
- 10.21105/joss.00825 is OK

MISSING DOIs

- None

INVALID DOIs

- None
trevorb1 commented 11 months ago

Hi @olejandro, I hope you are doing well! Thank you again for taking the time to review the paper. I am just checking in to see if you have any comments on the software side of things for this review? Thanks in advance!

fraukewiese commented 11 months ago

@olejandro : Thanks a lot for reviewing. Are you satisfied with the changes made and answers so far by @trevorb1 ?

olejandro commented 11 months ago

@trevorb1 @fraukewiese apologies for the slow response! Yes, I am generally satisfied with changes and answers so far. Thanks @trevorb1! One last comment on the paper that I feel would strengthen it is to expand the comparison with other open source frameworks (i.e. addressing my first point) with:

I'll come back with any comments on the software / repository towards the end of the week.

olejandro commented 11 months ago

@trevorb1 @fraukewiese I have mostly completed the checklist. I have experience a few issues with the software (I tried it on Windows):

Let me know if I should open an issue in the original repository for any of the above (or whether I am doing anything wrong).

One last point, it would be great if there was a URL to contributing guidelines included in the readme.

fraukewiese commented 11 months ago

@olejandro : Thank you very much for the thorough review.

trevorb1 commented 10 months ago

Hi @olejandro, thank you for the review! I just confirmed on my side that the tests are failing on Windows as well. Thanks for flagging this! I have just created a new issue for that here, as I am about to start working on it. Please feel free to add any information to the issue ticket, and/or create new issues for the other bugs you flagged. :)

trevorb1 commented 10 months ago

Thank you again for your feedback, @olejandro! Please find below point by point responses to your comments. If anything is unclear or needs further explanation, please let me know.

One last comment on the paper that I feel would strengthen it is to expand the comparison with other open source frameworks Apologies for the confusion on your original comment. Please find an update in this commit that states how otoole fits into the literature.

Some of the [Windows] tests fail Good catch! Windows tests have been updated in this PR. Additionally, Windows tests are now included in the CI.

An attempt to generate res.png results in FileNotFoundError: [WinError 2] "dot" not found in path. res.graphml does get generated; This seems to be a graphviz dependency issue, where it must be installed on the system. We have added instructions on how to do this for both Windows and UNIX in this PR. Please see here for the documentation updates.

Validation example leads to AttributeError: 'NoneType' object has no attribute 'keys' after validation starts. Unfortunately, I was not able to recreate this issue. I tried on both Windows and Linux, and the validation worked. On Windows, this is the command I used using otoole version 1.1.0.

otoole validate datafile data.txt config.yaml --validate_config validate.yaml 

If you are still experiencing an issue, maybe it will make sense to move this to its own issue ticket. In the issue ticket, can you please:

  1. Confirm the version of otoole you are running through the otoole --version command
  2. Please run the validation command in verbose mode (ie. otoole -v validate ...) to print out the traceback error.

it would be great if there was a URL to contributing guidelines included in the readme. Agreed! This has been updated in this PR; additionally, a wider selection of badges have been added to the readme.

trevorb1 commented 10 months ago

@editorialbot generate pdf

editorialbot commented 10 months ago

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

trevorb1 commented 10 months ago

@editorialbot check references

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

OK DOIs

- 10.1016/j.enpol.2011.06.033 is OK
- 10.12688/openreseurope.15461.1 is OK
- 10.1038/s41597-022-01737-0 is OK
- 10.1016/j.enpol.2011.09.039 is OK
- 10.1007/s12532-017-0130-5 is OK
- 10.5281/zenodo.6522795 is OK
- 10.1016/j.esr.2021.100650 is OK
- 10.1016/j.envsci.2022.07.007 is OK
- 10.1016/j.rser.2018.11.020 is OK
- 10.5334/jors.188 is OK
- 10.21105/joss.00825 is OK
- 10.12688/openreseurope.13633.2 is OK

MISSING DOIs

- None

INVALID DOIs

- https://doi.org/10.1016/j.softx.2021.100967 is INVALID because of 'https://doi.org/' prefix
trevorb1 commented 10 months ago

@editorialbot check references

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

OK DOIs

- 10.1016/j.enpol.2011.06.033 is OK
- 10.12688/openreseurope.15461.1 is OK
- 10.1038/s41597-022-01737-0 is OK
- 10.1016/j.enpol.2011.09.039 is OK
- 10.1007/s12532-017-0130-5 is OK
- 10.5281/zenodo.6522795 is OK
- 10.1016/j.esr.2021.100650 is OK
- 10.1016/j.envsci.2022.07.007 is OK
- 10.1016/j.rser.2018.11.020 is OK
- 10.5334/jors.188 is OK
- 10.21105/joss.00825 is OK
- 10.12688/openreseurope.13633.2 is OK
- 10.1016/j.softx.2021.100967 is OK

MISSING DOIs

- None

INVALID DOIs

- None
fraukewiese commented 9 months ago

@trevorb1 : Thanks for the updates :) @olejandro : Are you satisfied with the changes and replies to your comments/raised issues? And do you still experience this issue: "Validation example leads to AttributeError: 'NoneType' object has no attribute 'keys' after validation starts." (see @trevorb1 's explanation above.

olejandro commented 9 months ago

@trevorb1 @fraukewiese thanks for this! I have updated my checklist. I don't have any further comments. Great work!

fraukewiese commented 9 months ago

@olejandro and @fneum : Thank you so much for your great work in reviewing!

fraukewiese commented 9 months ago

@trevorb1 : There are a few minor issues on the paper:

fraukewiese commented 9 months ago

When these are corrected, could you then:

I can then move forward with accepting the submission.

trevorb1 commented 9 months ago

@editorialbot generate pdf

editorialbot commented 9 months ago

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

trevorb1 commented 9 months ago

@editorialbot check references

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

OK DOIs

- 10.1016/j.enpol.2011.06.033 is OK
- 10.12688/openreseurope.15461.1 is OK
- 10.1038/s41597-022-01737-0 is OK
- 10.1016/j.enpol.2011.09.039 is OK
- 10.1007/s12532-017-0130-5 is OK
- 10.5281/zenodo.6522795 is OK
- 10.1016/j.esr.2021.100650 is OK
- 10.1016/j.envsci.2022.07.007 is OK
- 10.1016/j.rser.2018.11.020 is OK
- 10.5334/jors.188 is OK
- 10.21105/joss.00825 is OK
- 10.12688/openreseurope.13633.2 is OK
- 10.1016/j.softx.2021.100967 is OK

MISSING DOIs

- None

INVALID DOIs

- None
trevorb1 commented 9 months ago

Hi @fraukewiese! Thank you for your final edits! All points have been addressed in this commit. On your comment for line 75/76, I instead did a slight reword just to make the sentence more clear, instead of adding a comma. Please let me know if the line is still unclear and I will update. Thank you again!