search

openjournals / joss-reviews

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

[REVIEW]: autumn: A Python library for dynamic modelling of captured CO~2~ cost potential curves #3203

Closed whedon closed 2 years ago

whedon commented 3 years ago

Submitting author: @Eugenio2192 (Eugenio Salvador Arellano Ruiz) Repository: https://gitlab.com/dlr-ve/autumn/ Version: v0.1.0 Editor: @timtroendle Reviewers: @igarizio, @milicag Archive: 10.5281/zenodo.5163098

: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/d7bab860686df296e35cc437f0630ff2"><img src="https://joss.theoj.org/papers/d7bab860686df296e35cc437f0630ff2/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/d7bab860686df296e35cc437f0630ff2/status.svg)](https://joss.theoj.org/papers/d7bab860686df296e35cc437f0630ff2)

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

@potterzot & @igarizio, 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 @timtroendle 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 ✨

Review checklist for @milicag

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @igarizio

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @potterzot, @igarizio 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 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.19 s (318.1 files/s, 42970.8 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          21            794           1502           2859
YAML                             7             22              9           1015
JSON                            10             10              0            359
reStructuredText                14            161            147            215
Markdown                         2             57              0            170
Jupyter Notebook                 2              0            569            150
TeX                              1             11              0            109
DOS Batch                        1              8              1             26
HTML                             1              1              0             24
make                             1              4              7              9
XML                              1              0              0              1
-------------------------------------------------------------------------------
SUM:                            61           1068           2235           4937
-------------------------------------------------------------------------------

Statistical information for the repository 'd46ee64810c51de729dc06f3' was
gathered on 2021/04/22.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Arellano Ruiz                    1             1              1            0.02
Arellano Ruiz, Eugen            22          4152           2818           62.52
Eugenio                          1          3497              0           31.37
Eugenio Arellano                11           340            187            4.73
Eugenio Salvador Are             4            62             37            0.89
ve-py@dlr.de                     2            41             13            0.48

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
Arellano Ruiz                 1          100.0          0.3                0.00
Arellano Ruiz, Eugen       3736           90.0          1.3                7.90
DLR                          40          100.0          0.2                5.00
Eugenio Arellano              3            0.9          0.7                0.00
Eugenio2192                1375          100.0          0.1                6.40
whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1039/C7EE02342A is OK
- 10.1007/s11356-016-6810-2 is OK
- 10.1016/j.esr.2018.11.004 is OK
- 10.1038/s41467-020-20015-4 is OK
- 10.1016/j.energy.2018.10.114 is OK
- 10.1016/j.energy.2018.06.222 is OK
- 10.18419/opus-2015 is OK
- 10.1016/j.epsr.2020.106690 is OK
- 10.1021/acs.est.5b03474 is OK

MISSING DOIs

- None

INVALID DOIs

- None
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:

timtroendle commented 3 years ago

@potterzot, @igarizio, as mentioned over in the pre-review, we are still operating in reduced service mode in which we ask reviewers to finish their review within six weeks at the latest. I'll add a reminder for both of you when half of that time has run up.

It's great if you can finish your review earlier than that, of course.

timtroendle commented 3 years ago

@whedon remind @potterzot in three weeks

whedon commented 3 years ago

Reminder set for @potterzot in three weeks

timtroendle commented 3 years ago

@whedon remind @igarizio in three weeks

whedon commented 3 years ago

Reminder set for @igarizio in three weeks

whedon commented 3 years ago

:wave: @potterzot, please update us on how your review is going (this is an automated reminder).

whedon commented 3 years ago

:wave: @igarizio, please update us on how your review is going (this is an automated reminder).

potterzot commented 3 years ago

I had some trouble installing cartopy that had to do with depreciated proj_api.h files, but I have everything running and should have a review done by the end of next week.

@timtroendle I cannot edit the checklist and it seems my invitation has expired, can you issue another one? Thanks!

timtroendle commented 3 years ago

@whedon re-invite @potterzot as reviewer

whedon commented 3 years ago

OK, the reviewer has been re-invited.

@potterzot please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

whedon commented 3 years ago

:wave: @potterzot, please update us on how your review is going (this is an automated reminder).

whedon commented 3 years ago

:wave: @igarizio, please update us on how your review is going (this is an automated reminder).

igarizio commented 3 years ago

👋 @Eugenio2192 Thank you for the submission. This is a super interesting topic! I have been reviewing your paper, code, and documentation and I have a few comments:

General checks:

Functionality

Documentation:

Other:

Extra comments

These are not really required for the review. I include them just in case they are helpful to you.

This is what I have so far, I will keep working on this. Please let me know if I made any mistake on any of this. Again, thank you for the submission. Have a great rest of the weekend!

Eugenio2192 commented 3 years ago

👋 @Eugenio2192 Thank you for the submission. This is a super interesting topic! I have been reviewing your paper, code, and documentation and I have a few comments:

General checks:

* **Contribution and authorship:** I see that you have made a lot of contributions to the software and I just wanted to check with you that the other listed authors had also made substantial contributions.

Functionality

* **Installation:**

  * It seems like the software is not yet listed on [pypi](https://pypi.org/search/?q=autumncpc) (you also mention this on the [README](https://gitlab.com/dlr-ve/autumn/-/blob/master/README.md)), this is not a problem at all, but I think it would be best not to put the `pip install autumncpc` instruction on your README/documentation until it has been properly listed. At the end of the review, your repo will be archived and it could happen that someone in the next few days takes the name _autumncpc_ from pypi (so your archived repo would be forever instructing to install someone else's code). [@timtroendle would you please correct me if I am wrong or if you have a different opinion, I am not 100% sure about this]
  * When following the other installation instructions, my console got flooded with this warning:
    > Warning : you have pip-installed dependencies in your environment file, but you do not list pip itself as one of your conda dependencies.  Conda may not use the correct pip to install your packages, and they may end up in the wrong place.  Please add an explicit pip dependency.  I'm adding one for you, but still nagging you.

    I waited 40 minutes and canceled the installation. I added _pip_ to the .env file and ran the installation again. With this, the warning disappeared and the installation finished in a couple of minutes. I am not certain that this was the issue (maybe I just got unlucky the first time), but this is something you might want to consider.
  * There seem to be some slight differences between the installation instructions listed on the [README](https://gitlab.com/dlr-ve/autumn/-/blob/master/README.md) and on the [documentation](https://autumn.readthedocs.io/en/master/installation.html). It would be great if you could make them the same so that your future users do not get confused.
  * Small detail: I think there is an extra _.py_ in `from autumn.scripts.create_data_tree.py import main_build_file_tree` ([here](https://autumn.readthedocs.io/en/master/getdata.html#getting-the-necessary-data))

Documentation:

* **A statement of need:** I was not able to find this in your README or documentation.

* **Installation instructions:** Same comment from the functionality section.

* **Example usage:**

  * I was not able to run [this code from the documentation](https://autumn.readthedocs.io/en/master/basic.html#scenario-representations) (I created [an issue for this](https://gitlab.com/dlr-ve/autumn/-/issues/1))
  * I was not able to run this line from the README:
    `scenarios = scenario_development_one_hot_encoded(["basic", "iron", "cement"])`
    I did not create an issue because it seems like `scenario_development_one_hot_encoded` is just missing an argument.
  * Small detail: On the README you define `Distribution = cost_captured_distribution("basic")`, but then use `distribution.data` (Distribution vs distribution). This also causes an error.

* **Functionality documentation:** Good documentation in general, but there seems to be a missing section [here](https://autumn.readthedocs.io/en/master/autumn.html#core) (I see you have the docstrings for that part, so maybe it is just an issue with sphinx or something related (?)).

* **Automated tests:**

  * I see that you had a folder with a couple of tests, but most of them (`test_common.py`, `test_core.py`, `test_data_operations.py`, `test_harmonization.py`, `test_powerplantmatchingcc.py`) were empty or did not really test any code (`test_harmonize.py`).
  * It would be useful for users if you could also add some instructions on how to run the tests (the typical `pytest ...` is ok)

* **Community guidelines:**  I was not able to find this in your README or documentation.

Other:

* **Spelling:** I am not great at this (English is not my first language), so consider them with caution.

  * Asses vs Assess: [line 50](https://gitlab.com/dlr-ve/autumn/-/blob/master/paper/paper.md#L50).
  * Visualizaion vs Visualization: [line 72](https://gitlab.com/dlr-ve/autumn/-/blob/master/paper/paper.md#L72).
  * Small detail: CO2 vs CO_2: [line 50](https://gitlab.com/dlr-ve/autumn/-/blob/master/paper/paper.md#L50). You consistently use CO_2 (with the subscript) throughout the paper, but here you used CO2.
  * There might be more, but these were the ones I was able to find.

Extra comments

These are not really required for the review. I include them just in case they are helpful to you.

* Use `os.path.join` instead of "**/**" with strings

* Use uppercase for constants (for example in `create_data_tree.py`)

* There is an empty `conftest.py` (I was not sure if this was on purpose or not)

* There are two README files with different information (I was not sure if this was on purpose or not)

This is what I have so far, I will keep working on this. Please let me know if I made any mistake on any of this. Again, thank you for the submission. Have a great rest of the weekend!

Thanks a lot for the feedback @igarizio most of the issues have been addressed in the latest commits. Some of the changes will take some more time but the critical ones like documentation consistency and tool functionality are resolved.

Regarding the participation of the secondary authors. I have been mostly in the task of translating their contributions into code. Dr. Fuchs is behind the technical oversight of the development of the tool, Mr. Wulff is behind the ideation of the application and reviewing of the changes that were implemented, both of them offered continuous colsuntancy and code reviews during the whole lifetime of the development. Mr. Wu is behind the theoretical background of the tool, an important part of his previous work was used as a base for this development and he also offered reviewing.

Regarding the test framework, it is still work in progress but we are keeping track of the tool health using coverage as an indicator. Hopefully we will get soon the chance of completing this in the following months.

Have a great week!

Eugenio

timtroendle commented 3 years ago

Hello @potterzot, could you please let us know where you stand with your review?

igarizio 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:

timtroendle commented 3 years ago

@potterzot, we need to finish the first round of reviews. Can you please let us know where you stand with your review?

igarizio commented 3 years ago

Hi @Eugenio2192! Thanks for all the changes you have already made.

I'm not 100% sure this is relevant for JOSS (maybe @timtroendle can tell us), but it could be to you. I was wondering if you had the proper rights for the data on your repo. I'm asking this because I saw CEPCI.csv and where you had obtained it. Again, I'm not sure if this should be relevant for me as a reviewer (also, I'm not a lawyer), but I'm just checking so that everyone can stay out of trouble. Additionally, please let me know on this thread once you are done with the test suite so that I can try it. Have a great week!

Eugenio2192 commented 3 years ago

Hi @Eugenio2192! Thanks for all the changes you have already made.

I'm not 100% sure this is relevant for JOSS (maybe @timtroendle can tell us), but it could be to you. I was wondering if you had the proper rights for the data on your repo. I'm asking this because I saw CEPCI.csv and where you had obtained it. Again, I'm not sure if this should be relevant for me as a reviewer (also, I'm not a lawyer), but I'm just checking so that everyone can stay out of trouble. Additionally, please let me know on this thread once you are done with the test suite so that I can try it. Have a great week!

Thank you for your remarks @igarizio

I understand your concern regarding the data in the repository, we took special care on this area. All the data provided is either produced directly by us or added as a placeholder. Regarding the CEPCI, you are right, it is a problem currently, I provided it in order to remove the overhead of implementing the tool, of course validated that the sources directly through the University library to make sure they were correct before doing so. but you are right on having this property concerns. However, the output data of the tool as it is, is not intended to be referenced directly, the idea is that whichever scientific data that is produced using this tool should go through a validation step before being introduced in the Potential Curves pipeline in order to get a valid output.

Also the Test suite is slowly coming together, I'll keep you updated.

Wishing you a great week as well

Notes on edit: I talked originally on a consideration that was made that was not actually implemented in the repository.

timtroendle commented 3 years ago

@igarizio, @Eugenio2192, thanks for pointing out the potential copyright issue with one of the data sets. From a JOSS perspective, it's our task to make sure that you are providing your tool with an open license. Checking whether you are legally allowed to do so is beyond our scope. As @igarizio already pointed out, we cannot give any legal advise.

To keep you up to date with the status of the review: I am currently searching for a second reviewer and I will let you know as soon as we have a second reviewer for your submission.

Eugenio2192 commented 3 years ago

@timtroendle @igarizio It was decided to prune the CEPCI file from the repo, an instruction for an user to get their own CEPCI values was included. This is to avoid any potential property issues.

timtroendle commented 3 years ago

:wave: @potterzot, as I haven't heard back from you in a while, I will remove you from the list of reviewers.

timtroendle commented 3 years ago

@whedon remove @potterzot as reviewer

whedon commented 3 years ago

OK, @potterzot is no longer a reviewer

timtroendle commented 3 years ago

@whedon add @milicag as reviewer

whedon commented 3 years ago

OK, @milicag is now a reviewer

timtroendle commented 3 years ago

Thanks a lot for accepting to jump in as a second reviewer, @milicag. Please let me know if you need any help in the process.

milicag commented 3 years ago

@Eugenio2192 Thank you for creating very interesting code for an important topic. Please take a look at the issues I created that have been pending for a while now directly on your repository. After I get the responses I will be able to move on with the review. Thank you!

Eugenio2192 commented 3 years ago

@Eugenio2192 Thank you for creating very interesting code for an important topic. Please take a look at the issues I created that have been pending for a while now directly on your repository. After I get the responses I will be able to move on with the review. Thank you!

Thanks for jumping @milicag I'm sorry for the delay on the issue solving(and test building). Last month has been very loaded and I'm currently the only active mantainer available. I will get the issues solved ASAP.

Eugenio2192 commented 3 years ago

The raised issues have been solved, the package should be running properly now. Regarding the testing, Is just that a lot of the core functions do geographical operations from packages like geopandas, this can be very tricky to get running in the GitLab Runner, because they are based on libraries whose binaries have to be built or adquired outside of python. Long story short, I have been hitting a lot of bumps and Im coming up with workarounds, but it takes some time; but I don't think this interferes with the core functionality of the package.

Eugenio2192 commented 3 years ago

I also highly recommend using the Binder instance for running the Notebooks for functionality tests, the setup takes some time the first run but once there is a running image it works with little issues: https://mybinder.org/v2/gl/dlr-ve%2Fautumn/HEAD?filepath=notebooks%2FDemostration.ipynb (You might get an issue with key validations, it can be solved by simply restarting the kernel)

milicag commented 2 years ago

Thanks @Eugenio2192, I'll take a look over the weekend and keep you posted. Regarding the testing, I was thinking more like a unit testing suite to ensure proper implementation of all the equations, or say at least some percentage of the functionality, with a simple assertion to pass or fail the test. This is also useful for any future development, as reference points. It does not need to be automated on any online service. If you have a part of it in a notebook, I'm pretty sure it can be transferred to a folder named tests, and then have corresponding test modules test_nameofmyfuntionalmodule.py. You could then run those tests each time new functionality is added. Please see if this is feasible to some extent and let me know.

milicag commented 2 years ago

@Eugenio2192. Thank you for addressing the issues. Please find and address some minor notes I left in our now closed review issues. I also opened up a new issue for a few typos I was able to find. Please go through the notebooks, code, and documentation for any final touch-ups with respect to formatting and typos. I found out that geopandas version 0.9.0 will throw an error related to not having attribute GeoDataFrame. It may be useful to have the geopandas version specified strictly in the version you intend to tag for that reason.

Eugenio2192 commented 2 years ago

@milicag I addressed the issues in the repository. Also added an explicit tests section in the documentation instructing the user to proof their installation by running the Demostration notebook. Regarding the geopandas issue, I added a strict version in the environment files to avoid that issue.

milicag commented 2 years ago

@Eugenio2192 Great, thanks. If the editor agrees, I'd like to have a final look on Saturday and I'll let you know once that is done.

timtroendle commented 2 years ago

Of course. Thanks, @milicag.

milicag commented 2 years ago

@Eugenio2192 I can't find where you've addressed my suggested installation instructions from this issue thread: https://gitlab.com/dlr-ve/autumn/-/issues/2. That would be very practical and is usually required by JOSS. In my experience the users really like to have a set of commands they can just copy over and run, without knowing a whole lot about the packages themselves. If you send people to each corresponding package website, you are really loosing some potential users. I hope this may convince you to introduce the edit in the installation instructions. That would be my last request, the paper looks good otherwise.

timtroendle commented 2 years ago

@igarizio , could you please update us on your remaining concerns? Are they resolved?

igarizio commented 2 years ago

@timtroendle My apologies for the delay. Everything has been resolved ✔️

accon commented 2 years ago

Hey all, this is Niklas Wulff, co-authoring the publication. Since Eugenio currently is in Mexico, resolving the still standing issues was delayed. Apologies for this, he can now maintain the repo also from abroad. AFAI can see, the only open issue regards the list of dependencies under install instructions right?

timtroendle commented 2 years ago

That's correct.

Eugenio2192 commented 2 years ago

The dependencies have been listed in the environment.yml (for conda users) and requirements.txt (for pip users). The installation instructions were refactored to be more clear including instructions referring to jupyter notebooks. The issues raised by @milicag in the repository including his suggestion to handle the CEPCI data were handled.

Let us know if there is anything else to be solved.

milicag commented 2 years ago

One last ask is to add a line on how to activate the environment, as then not everyone needs to be familiar with the concept to use the tools. In addition, limiting the version of geopandas in the yml would lead to more smooth installation behavior for more users. I've already checked the last mark. Thanks for writing interesting code!

Eugenio2192 commented 2 years ago

I added geopandas but also cartopy versions to the reqs, also reordered a little the instructions so they have logical execution order. @milicag and @igarizio Thanks for the very extensive review, we really learnt a lot from it.

timtroendle commented 2 years ago

@whedon generate pdf