Closed editorialbot closed 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
Software report:
github.com/AlDanial/cloc v 1.88 T=0.33 s (151.5 files/s, 63758.5 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
Python 22 2511 6128 7678
HTML 13 160 0 2804
TeX 8 257 5 933
Markdown 3 61 0 256
CSS 1 25 5 150
XML 1 2 0 35
YAML 1 1 4 18
Bourne Shell 1 3 1 4
-------------------------------------------------------------------------------
SUM: 50 3020 6143 11878
-------------------------------------------------------------------------------
gitinspector failed to run statistical information for the repository
Wordcount for paper.md
is 1471
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1109/FIE.2016.7757589 is OK
- 10.1021/ie4033999 is OK
MISSING DOIs
- None
INVALID DOIs
- https://peer.asee.org/28757 is INVALID because of 'https://doi.org/' prefix
- 10.18260/1-232083 is INVALID
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
My review is complete.
The authors have produced and described pyromat
, a utility for calculation of gas-phase (and, to a certain extent, mixed gas/liquid) thermodynamic properties, based on established models. They provide data for ~1000 unique species, which include both common and uncommon gas-phase molecules and ions. The code is written clearly, and the documentation is, for the most part, quite good. While the use case for pyromat
in research seems unclear to me (note: I do not study gas-phase or combustion thermochemistry), this seems an extremely necessary tool for students of chemistry/chemical engineering, as well as industrial engineers. My comments are below.
Compliments:
Minor issues:
pyromat
.Major issues:
apps/cycle.py
, for instance, the RefrigerationCycle, OttoCycle, and DieselCycle classes are defined but not implemented.Thank you very much for your time in the review. I'd just like to clarify the two major issues:
tests-jranalli
. Specifically the files test_ig
and test_mp1
. Is there some type of documentation of that we could add that would help to satisfy this concern?Hi @espottesmith, great job on the review! Your efforts and haste are really appreciated.
@jranalli, regarding the comments of @espottesmith...
BTW, the 2nd reviewer requested a few week delay in starting since they were on vacay. I expect they'll be dropping by to add their review shortly, so hopefully you can work on the current suggestions.
Thanks very much for your work. I'll make a few comments here:
With Respect to Test Suites This idea really needs to be divided into two parts: (1) the testing required to validate a new data set that is imported into the suite, and (2) testing required to validate new class functionality or new features.
(1) New data are carefully validated against reference data provided by the same sources from which the models are entered. This is a time-consuming process - for multi-phase source data, it has to be done manually for every data source, since the raw data are being pulled from publications. However, once validated, the work is complete, and does not need to be repeated with new releases, so no automation suite is used.
We deliberately do not currently publish validation data. In older versions of the code, I used to include records of my validation work, but I have received emails from private companies that were attempting to interpret this as a warranty of some kind. My official policy is that the sources of the original data are cited, and users should always validate their calculations against their own experimental data in the context of their application.
However, the codes that were used to validate the original ideal gas model data are actually still coded directly into the classes. The ig
class has a method called _ig__test()
, and ig2
has _test()
. The ig
substances even have validation data tables embedded directly in their raw data. _ig__test()
now complains about errors because it has not been updated along with major version changes, but it still successfully validates the original source data. This has even been used to demonstrate continuity errors in NIST's source data.
(2) On this front, @jranalli deserves credit for making similar arguments for some time. In brief, this is a proposal that makes sense, but to implement it well, it would require an effort at least as difficult as writing the package itself. Now that @jranalli is contributing so heavily to the project, it may make sense to include his validation suites in the main branch.
The challenge is a familiar one: the number of permutations of input/output possibilities is now so large (e.g. array/scalar, liquid/mixed/vapor/super-critical, primary/inverse properties, near limits/centered, countless permutations of units, and all of the combinations of those), that historically, hard-coded test suites have not performed well enough to justify the effort. So far, our most successful model has been to run ad-hoc targeted tests to try to break the new functionality, then @jranalli tries harder with his own test suite, then we release to the community. This workflow has functioned pretty well so far, but we are always happy to improve.
Undocumented Partially Coded Modules
True - we do have some undocumented code in the master branch. It is not currently part of package functionality. The apps
module is utterly undocumented (except in-line) and is not imported by default. That module actually serves a specific (if slightly secret) purpose for our efforts to port to a web-based interface for PYroMat. The code there is part of a (still live) proof-of-concept portion of the website. It will be cleaned up in later releases, but we have decided to leave it as an easter egg for any interested users who may want to do their own developing.
LICENSE True. That will be easily corrected.
README failure
This is actually a change in the functionality of info()
in the last release. The new behavior was documented on the website, and in the handbook (see sections 2.2.3 and 2.2.4), but I forgot to update the README. Oops. Good catch!
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Fixes implemented in the repository:
apps
folder) to a separate branch to eliminate confusion.Quick update from me @jgostick, @jranalli: I will start working on the review this week and hope to finish it on the weekend.
Updates to the text of the paper based on comments from @fwitte. Rebuilding
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Thanks for already answering my initial comments, I still have some small parts to do, which I could not finish to work on last weekend. Once I have done that, I will give my review here.
Hey all, how are things progressing here?
Thank you for reminding me @jgostick and thank you for your patience @jranalli. My schedule was quite packed, I want to check two more features of the package (mixtures and the water IF97) to finalize. I will let you know here after that. I hope I can fit that into my weekend.
FYI, the if97
class is deprecated. The mp1
class is now used by all multi-phase substances (including water).
But it uses the IF97 equations, right? I was using the mp1 class and understood it was an implementation of the IF97 equations.
Good question. mp1
uses a generalized "Span and Wagner" equation of state that applies over both liquid and vapor phases without piece-wise transitions. If you are curious, the mp1
model is developed in some detail in chapter 7 (pg 97-114) of the PYroMat handbook.
The if97
class uses the IF97 piece-wise formulation that you probably know already. The IAPWS actually published an earlier model in 1995 using the Span and Wagner formulation, and it was updated in 2018.
The IF97 is handy for engineering use, because it includes polynomial expansions good for "inverse" property calculations, so it was the first thing I tried. If I had to write a code to model a Rankine cycle from scratch, I'd use IF97. However, Span and Wagner equations of state are now almost universal in applications like PYroMat because:
@chmarti1, thank you very much for the detailed explanation. I was wondering, why I could not replicate some of the bugs in IF97, which seem to exist in both, CoolProp and the IAPWS package, also see https://github.com/CoolProp/CoolProp/issues/1918.
@jgostick: Just wanted to add my checklist, do you know, where it has gone?
Nevermind, copy pasted the other one.
@jranalli, @chmarti1: Thank you for your replies and in depth explanation. Here are a few lines for my review:
Overall, I find this a very useful package. To my knowledge, the NASA polynomes are not included in other open source packages like thermo, CoolProp and iapws, but only in EES or other proprietary software. Especially the extensive handbook is well written and contains a lot of information helping both, people learning thermodynamics and those, who have been working with unit based simulation but need a refresher in fluid property modeling.
The online documentation is also well written and features some small gimmicks with the live apps, giving potential users a quick idea, what the software does. I have some suggestions for the online documentation, which I listed under ideas/suggestions, which are not mandatory for the paper to be accepted.
All major issues have been fixed already, see the comments of @espottesmith
.hpd
file, give it a new id and some mixture contents, this might be hard to find for less experienced users and you are limited to a somewhat fixed mixture composition. It would be cool, if the user could build his or her own mixtures by providing the molar or mass based mixture contents and it does not seem too hard to implement.I am not sure, how complicated it is to include the last two features in your documentation. I do very much like using sphinx (with readthedocs if you do not want to self-host) as it is very easy and convienient to include these.
Recommend accepting the paper as is after the last two issues have been resolved.
Thanks for your work on the review.
extra_requires
Definitely appreciate the suggestions and we'll consider those as we move forward.
Thanks again to reviewers for you time! As of now, pull requests have been implemented regarding all the major and minor issues that had been raised by reviewers. Not sure how the process continues, but if anything else is needed from us, please just speak up!
Great stuff everyone...the above review thread shows a very comprehensive review and revision process indeed. I have confirmed that via the discussion and various merged PRs that all of the reviewers comments and concerns have been addressed. So I will move forward with accepting it. There few housekeeping items to get to first though. I will paste a checklist below.
@editorialbot set <DOI here> as archive
@editorialbot set <version here> as version
@editorialbot generate pdf
@editorialbot check references
and ask author(s) to update as needed@editorialbot recommend-accept
Ok so I don't know how to mark the checkboxes, but I have double checked that everybody's ORCID and affiliation appears correctly on the paper, the version release number is 2.2.4 and it is uploaded to Zenodo with a DOI of https://doi.org/10.5281/zenodo.7262173
@editorialbot set 2.2.4 as version
Done! version is now 2.2.4
@editorialbot set 10.5281/zenodo.7262173 as archive
Done! Archive is now 10.5281/zenodo.7262173
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Authorship and affiliations all match with expectations. The title on the proof is correct. ORCIDs are all correct as well.
@jranalli is there any reason why Jacob Moore is on the paper but not on the repo or listed in the zenodo archive? Also, we like to have orcids on the zenodo archive as well...can you see about linking these?
He has been a contributor to the overall project for years but hasn't made code contributions and so doesn't show on Github. We looked at manually adding an author to the Zenodo archive, but it didn't look like there was an easy way to do so. We can take a look at whether there's a different method to do so, unless you have some suggestion that we didn't think of?
It's very easy to add an author to Zenodo. See the section labeled "How can I edit the metadata of a published record?" in https://help.zenodo.org for how to do this.
@jranalli: Usually, you can go to zenodo, click on edit publication (only the person who uploaded that publication) and then enter the names of the researches. Looks like so:
@chmarti1 created the archive, so I'll ask him to take a look.
@editorialbot check references
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1109/FIE.2016.7757589 is OK
- 10.1021/ie4033999 is OK
- 10.1038/s41586-020-2649-2 is OK
MISSING DOIs
- None
INVALID DOIs
- https://peer.asee.org/28757 is INVALID because of 'https://doi.org/' prefix
- 10.18260/1-232083 is INVALID
Hi @jranalli, the references in the pdf need some work. In addition to the issues pointed out above, there are many references missing their doi. 3 of the last 4 in the list have dois at the bottom of the page: https://doi.org/10.1023/A:1022362231796. Not sure about the NASA one, but can you take a look?
Also, for the nist paper, what about: https://doi.org/10.1021/acs.iecr.2c01427 instead of just linking to the nist website
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Submitting author: !--author-handle-->@jranalli<!--end-author-handle-- (Joseph Ranalli) Repository: https://github.com/chmarti1/PYroMat Branch with paper.md (empty if default branch): joss2022 Version: 2.2.4 Editor: !--editor-->@jgostick<!--end-editor-- Reviewers: @espottesmith, @fwitte Archive: 10.5281/zenodo.7262173
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
@espottesmith & @fwitte, 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 @jgostick 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 @espottesmith