-
-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[REVIEW]: Osier: A Python package for multi-objective energy system optimization #6919
Comments
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:
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
|
Software report:
Commit count by author:
|
|
Paper file info: 📄 Wordcount for ✅ The paper includes a |
License info: ✅ License found: |
Review checklist for @fredshoneConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
Really nice work well presented. I require some changes detailed below with optional actions mixed in. A lot of these are based on compatability with Windows. Although not required I suggest both adding Windows to your CI and including env control in the docs. Mamba is very good. There are example projects here and here. Installation and Installation Instructions: On a Windows machine I hit two blockers; (i) python version (issue here), and (ii) solver installation documentation (issue here). I strongly recommend adding a windows machine to the CI. Otherwise this is likely to reoccur. I have also suggested that the default Windows solver be glpk. This is easy to install and will smooth CI. It is also more in keeping with JOSS (in my opinion). if you stick with a less open solver I think this needs to be explicitly mentioned in ther paper. OPTIONAL, I also encourage use of mamba in the install instructions as per this issue. Example usage: OPTIONAL - there is only a single example of multi-criteria and it is a little hidden at the end of a notebook. Please consider making this more obvious. At the moment multi-criteria appears as an after-thought rather than a core feature. OPTIONAL - please consider making the example notebooks more accessible in the project rather than just docs. See this issue. Automated tests: Please add a code coverage report (eg pytest-cov). State of the field and References: Please provide some more specific description and reference to similar tools. I think your focus on multi-criteria is very important, as you state - there are many other open energy system modelling tools. You would make the case better by more explicit comparison to similar projects that don't do multi-criteria. For example calliope is very similar in many ways but does not to multi-criteria without hacking the solver. |
Hi @fredshone, I appreciate the review and the kind words. Addressing these issues is on my radar but I've hit a busy moment with other work -- I haven't forgotten about this, it's just on the back burner for now! |
No problem. You don't have to address everything at once - also happy to review bits and bobs when you have time. Similarly the review process is supposed to be open and two-way, some of my requests are quite specific, but you might be able to address them in better ways. Hope that makes sense. |
Hi @victoraalves, I hope you are doing well. I see you have not created a reviewer checklist yet. Could you please do so soon? If I can help you with anything, just let me know. |
Hi @victoraalves, could you please start the review? It has been two months since it started. I understand you are busy with other commitments. If you could provide some timeline, that would be helpful. Best wishes, Prashant |
Hi @prashjha firstly, sorry for the late reply. I will finalize my review by no later than next week. Again, my apologies. |
Review checklist for @victoraalvesConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
Glad to hear from you and thank you for the update. |
I have performed my initial review. The tool is interesting and a good contribution! Congratulations, nice work. I had some issues regarding Python version compatibility (already mentioned here by the other reviewer), as well as problems related to numpy 2.0 that breaks some of the functionality of the tool. InstallationInstallation on macOS was a bit problematic, due to the new Python versions and the Numpy version installed by default when doing "pip install". This needs to be clarified in the documentation and/or fixed in the project as well. TestingSome tests failed due to the lack of the "cbc" solver, despite the fact that I followed the installation instructions as recommended. EDIT: Now I found out that this (and other solvers) need to be installed separately. My suggestion then is to have a conda-based installation of your package, which will help to automate all of the installation process. Status of the fieldAs mentioned, I believe it's a good idea to highlight other tools that have the same (or similar) objectives and explain how Osier differentiates itself. |
Thank you both for your reviews! @fredshone, thank you for making a pull request, as well. Regarding the status of the field, how much elaboration are you both looking for? The paper is already ~1000 words. I have an open pull request that adds a direct reference to two open source packages. Is more elaboration necessary or will this suffice?
Please let me know! |
@samgdotson short is good I agree. (from memory) I'd like to see specific mention on how your work builds on above by adding multi-objective functionality. This could be a one-liner. FYI, I spoke to calliope authors and they agreed MO was not a proper or intended functionality. So lean on it hard I suggest. More pressing for me personally is that when I went through the examples - the (single) multi-criteria example was at the end of a long notebook. I'd like to see it (perhaps with an even more minimal example) brought more front and center so that a new user (me in this case) can more quickly use the examples to understand the project scope. |
Nice job with the CI and operating systems btw 🥇 |
@samgdotson Thank you for addressing this. I agree that you don't have to craft a long discussion on this. To me, your current PR is sufficient. |
@fredshone @victoraalves I pushed a final PR that should address the remaining comments. This PR
If you both agree and are satisfied, what are the next steps? Thanks for your reviews! |
looks good, i will make a final skim through everything once merged. |
@fredshone, all merged! |
@editorialbot generate pdf |
@editorialbot commands |
Hello @fredshone, here are the things you can ask me to do:
|
@editorialbot check references |
|
@samgdotson @prashjha - Looks ready to me, nice job 🙇🏻 . |
@fredshone, thank you for letting us know. |
Hi @victoraalves, I'm just checking to see if you are close to finishing the reviews! Thank you! |
HI @prashjha LGTM, nice work! |
@prashjha what are the next steps? |
Hi @fredshone and @victoraalves, thank you for your efforts in reviewing this submission! |
Hi @samgdotson, sorry for the delay. I am reading the draft and will let you know if I have any suggestions. Next step would be recommend accept and one of the EiC will make the final call. It should be fairly quick. Meanwhile, could you please archive (if not done already) the release using zenodo and provide the archive reference so that I can associate it with your JOSS submission? Also, please ensure that the zenodo archive's title matches the title of this JOSS article. If you have updated a version of your code, let me know, and I can update it here. |
@editorialbot generate pdf |
1 similar comment
@editorialbot generate pdf |
1 similar comment
Hello @samgdotson, I read the article and it looks good to me. Please read my comments above and respond. |
Hi @prashjha, here is the Zenodo publication is available here: 10.5281/zenodo.14216170 The current version of the code is v0.3.1, thanks! Let me know if I need to do anything else. |
@editorialbot set 10.5281/zenodo.14216170 as archive |
Done! archive is now 10.5281/zenodo.14216170 |
@editorialbot set v0.3.1 as version |
Done! version is now v0.3.1 |
Thanks, @samgdotson! |
@editorialbot recommend-accept |
|
|
👋 @openjournals/pe-eics, this paper is ready to be accepted and published. Check final proof 👉📄 Download article If the paper PDF and the deposit XML files look good in openjournals/joss-papers#6185, then you can now move forward with accepting the submission by compiling again with the command |
Submitting author: @samgdotson (Samuel Dotson)
Repository: https://github.com/arfc/osier
Branch with paper.md (empty if default branch): joss
Version: v0.3.1
Editor: @prashjha
Reviewers: @fredshone, @victoraalves
Archive: 10.5281/zenodo.14216170
Status
Status badge code:
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
@fredshone & @victoraalves, 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:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @prashjha 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 @fredshone
📝 Checklist for @victoraalves
The text was updated successfully, but these errors were encountered: