search

openjournals / joss-reviews

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

[REVIEW]: Desalination and brine treatment systems integrated modelling framework: simulation and evaluation of water and resource recovery #7062

Open editorialbot opened 3 months ago

editorialbot commented 3 months ago

Submitting author: !--author-handle-->@rodoulak<!--end-author-handle-- (Rodoula Ktori) Repository: https://github.com/rodoulak/Desalination-and-Brine-Treatment-Simulation-.git Branch with paper.md (empty if default branch): main Version: DesalSim 1.0.1 (https://pypi.org/project/DesalSim/) Editor: !--editor-->@elbeejay<!--end-editor-- Reviewers: @rkingsbury, @yalinli2, @BenWinchester Archive: Pending

Status

status

Status badge code:

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

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

@rkingsbury & @yalinli2 & @BenWinchester, 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 @elbeejay 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 @yalinli2

📝 Checklist for @BenWinchester

📝 Checklist for @rkingsbury

editorialbot commented 3 months 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 3 months ago

Software report:

github.com/AlDanial/cloc v 1.90  T=0.06 s (847.2 files/s, 187688.7 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          36           1436           2025           4421
Markdown                        15            565              0           3096
TeX                              2              3              0            196
-------------------------------------------------------------------------------
SUM:                            53           2004           2025           7713
-------------------------------------------------------------------------------

Commit count by author:

   189  rodoulak
    45  Rodoula Ktori
editorialbot commented 3 months ago

Paper file info:

📄 Wordcount for paper.md is 850

✅ The paper includes a Statement of need section

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

OK DOIs

- 10.1016/j.desal.2010.10.035 is OK
- 10.1016/j.renene.2011.03.040 is OK
- 10.1021/acssuschemeng.2c06636 is OK
- 10.1016/j.desal.2020.114928 is OK
- 10.3390/membranes6010003 is OK
- 10.1016/B978-0-444-50810-2.50018-X is OK
- 10.1016/j.desal.2013.03.033 is OK
- 10.36227/techrxiv.170595103.36689587/v1 is OK
- 10.1016/j.jclepro.2020.122472 is OK
- 10.1016/j.desal.2022.116005 is OK
- 10.1016/j.desal.2018.11.018 is OK
- 10.1016/j.jece.2021.105338 is OK
- 10.3390/separations9100295 is OK
- 10.1080/19443994.2014.948660 is OK
- 10.1016/j.desal.2024.117562 is OK

MISSING DOIs

- No DOI given, and none found for title: Plant design and economics for chemical engineers
- No DOI given, and none found for title: Deliverable 3.1 Report on the design procedure inc...
- No DOI given, and none found for title: WAVE Water Treatment Design Software
- 10.36227/techrxiv.170594775.55495325/v1 may be a valid DOI for title: Designing for the future: A Value-Sensitive Approa...

INVALID DOIs

- None
editorialbot commented 3 months ago

License info:

✅ License found: MIT License (Valid open source OSI approved license)

editorialbot commented 3 months ago

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

elbeejay commented 3 months ago

@rkingsbury, @yalinli2, @BenWinchester thanks for agreeing to review for JOSS. Instructions for generating your reviewer checklist and conducting your review are in the first comment of this issue thread, but feel free to reach out if you have any questions.

The review process at JOSS is quite fluid, you don't need to do a whole review at once, you are free to tackle this piece-by-piece if you choose, and converse with the author as needed.

Similarly, we ask reviewers to try and complete their reviews within 6 weeks (so by Sept 10 in this case), but we can also be flexible and accommodate extensions. All I ask is that you just communicate your plan clearly with us here so the authors and the other reviewers here understand what is going on. Especially with summer holidays I understand that folks may be away, so please just keep us informed, thank you!

yalinli2 commented 3 months ago

Review checklist for @yalinli2

Conflict of interest

Code of Conduct

General checks

Source codes available.

MIT license.

All commits were from rodoulak. The paper includes additional contributors.

The package desalsim contains simulation models for desalination and brine treatment technologies including nanofiltration/reverse osmosis, multi-effect distillation, thermal crystallization, chemical precipitation, eutectic freeze crystallization, and electrodialysis with membrane.

Though the mathematical development of these models have been described elsewhere, the Python implementation would be useful to researchers.

The paper contains no original data.

The paper contains no original results (there are tutorials, examples, and tests included in the package, see comments below on them).

The paper contains no data on humans subjects or animals.

Functionality

pip install desalsim worked, but pip install -r requirements.txt didn't, requirements.txt file missing.

The examples and tutorials largely worked, but some of them need to be updated. E.g., RuntimeWarning triggered for the default values in example 1: image

In some tutorials, Desalsim is used instead of desalsim, led to some importing errors.

Other minor errors in the tutorials that prevent new users to directly run them, e.g.,

Qf_nf was not defined in

https://github.com/rodoulak/desalsim/blob/8a2dc8dbf7d46574e23c3cf97259fa61c2d483e7/example/Example_1_Tutorial.md?plain=1#L118

Should this be just Qf?

I'm not sure if there are packages that can test .md files, but if the tutorials are in Jupyter Notebook, we can use automated tests.

There are no performance claims of the software.

Documentation

Motivation/purpose statement included in README and the paper.

Installation instructions are provided, though in some places Desalsim should be replaced with desalsim.

Examples included, but the authors are recommended to double-check the tutorials to make sure they've been updated throughout. Would be good to add automated tests.

A separate readthedocs page is available.

Test files available, but recommend renaming the scripts (e.g., start with test_) so that they can be auto-detected by pytest. Also, one of tests failed: image

Short description on how to contribute included in README.

Software paper

The paper indicated a general lack of OSS for simulation of desalination and minerals recovery systems. No comparison was provided.

The paper was well written. Only a possible misspelling (valisated should probably be validated). Also maybe want to capitalize the "resource" in the tags.

There are some issues in the mathematical description:

Please note that I didn't check the actual equations - I assume they and their implementation in the package are all correct. I'm not a desal person, so I'm not aware if there are good benchmark models that can be used to test the accurate of certain models. Maybe other reviewers have insight.

A separate bibliography file is included, looks good as in the proof.

Comments to be addressed:

BenWinchester commented 3 months ago

Review checklist for @BenWinchester

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

rkingsbury commented 3 months ago

Review checklist for @rkingsbury

Conflict of interest

Code of Conduct

General checks

Functionality

Installation works without problems. I have open an issue with some suggestions to further streamline the process for users and help the package adhere to Python best practices.

Note that the line pip install -r requirements.txt is no longer up to date, since dependencies are now included in setup.py. This should be replaced with pip install e desalsim I think.

Given the stated purpose of desalsim to analyze brine solutions, I have serious concerns about the way osmotic pressure is calculated in the NF and RO models (see Eq. A9 and this issue)

Apart from this potential source of inaccuracy, the examples provided show that desalsim incorporates both process simulation (returning mass flows, etc.) and economic evaluation (estimating CAPEX, OPEX, etc.).

No performance claims

Documentation

The README contains a clear and concise description of the software's purpose

Installation is handled by pip; dependencies are listed in setup.py. I was able to successfully install and run the package with pip.

However, when I tried to install a local ,editable version from git pip install -e <github repo folder>, I could not get the package to install correctly. Even though pip show desalsim showed the package installed at the correct location, I would receive an ImportError when trying to import it. This may be a packaging issue.

Yes, the package includes a thoroughly explained example of a specific treatment train. I was able to run both example python scripts without issue.

The Tutorial.md file provides a nice summary of the models included and their inputs and outputs. However, it does not provide comprehensive API documentation. Although Table 1 explains conceptually what the inputs and outputs are, it does not explain exactly how to set or retrieve these quantities. For example, at the end of the tutorial, the example calls the OPEX, sum_el_en, and sum_th_en attributes of the simulation results, but there is nowhere (that I can find) that explains that these are attributes of the simulation.

In addition, it seems that most of what is described in the Tutorials (Tutorial.md and the two example notebooks) is scripting rather than the functionality contained in desalsim itself. These tutorials are well written, and would be valuable resources for someone relatively new to python that wants to analyze a desalination / brine treatment train; however after reading them It is not clear to me what desalsim can calculate on its own vs. what needs to be scripted manually. As a concrete example, the definition of class indic at the end of one of the tutorials provide a nice way to visualize the results, but this seems like the type of thing that would have been included in desalsim itself rather than depending on the user to code a results summary class.

UPDATE - I see now that there is a readthedocs page that largely addresses the concerns above; it is considerably more comprehensive than the tutorials. However, this documentation is not referenced anywhere in the repository that I can see, and I only found it thanks to @yalinli2 comment. I think it's very important that the documentation page be linked from the README and/or the Tutorials in the repo. See Issue #12 in the repo.

UPDATE: above comments have been addressed.

The package contains comprehensive test coverage (95%), as indicated by pytest --cov tests/

---------- coverage: platform linux, python 3.10.12-final-0 ----------
Name                           Stmts   Miss  Cover
--------------------------------------------------
tests/__init__.py                 16      0   100%
tests/scaleup.py                  10     10     0%
tests/test_economic.py            35      1    97%
tests/test_ed.py                  52      1    98%
tests/test_edbm.py                92      1    99%
tests/test_med.py                 19      1    95%
tests/test_mfpfr.py               39      1    97%
tests/test_nanofiltration.py      23      1    96%
tests/test_thermal_cryst.py       62      1    98%
--------------------------------------------------
TOTAL                            348     17    95%

The README contains a brief section inviting contributions and describing basic steps in the GitHub workflow.

Software paper

~~The paper lacks a bit of specificity in the Statement of Need and the Limitations section about what type of modeling is and is not included. This lack of clarity is signaled by the sentence "The proposed software is not intended to be a substitute for sophisticated physical models or a system dynamics approach." ~~

~~Clearly a major part of the motivation is TEA modeling, but there is also obviously some level of process modeling here. It would be helpful to clarify this in more detail, esp. given that the availability of other tools that have different strengths (see comments below on that point). ~~

The description given in the first two paragraphs of the README is clearer (and the figure helps as well) about exactly what this package does. I suggest both be incorporated into the paper itself.

UPDATE: above comments have been addressed

This aspect requires some improvement. First, at line 35: The reference for desalination TEA models is from 2002. Considering all the activity in this field of late, is there a more up to date reference you could use?

Related, line 36: I feel your statement of need should mention the U.S. Dept. of Energy's WaterTAP, which is a significant, recent, open-access effort to develop a process simulation platform for water desalination and brine treatment. I think your contribution is valuable and distinct, but it would be helpful to the "average" reader for you to briefly (1 sentence) explain why they might choose to use DesalSim and not WaterTAP. (For example, different model coverage, simpler to learn, etc.)

UPDATE: above comments have been addressed

Generally yes, however a few typos or unclear terms should be corrected:

UPDATE: above comments have been addressed

The list of references is complete. However, there appear to be many references in the Ref List which are not cited in the paper. Some of these appear to be cited in the Mathematical description .pdf, but that file has its own reference list.

Related - Thanks to @yalinli2 's comments I see that there is an additional pdf called Mathematical Description, but I do not see this pdf referenced anywhere in the main paper or in the documentation. What is it's intended purpose?

UPDATE: above comments have been addressed

elbeejay commented 3 months ago

Hello @yalinli2, @BenWinchester, and @rkingsbury - we are about 3 weeks in, so halfway, through the standard review time period. Just trying to ping you all so this doesn't slip off of to-do lists.

Again, please let us know here if you need an extension for whatever reason.

We appreciate open-communications as we attempt to keep the JOSS review process as transparent and friendly (to all involved) as possible.

BenWinchester commented 3 months ago

@elbeejay , yes, I should have it done by the end of next week—am currently on annual leave ⛱️

yalinli2 commented 3 months ago

@elbeejay thanks for the reminder, I also plan to work on it in the next week or so... I've been in the travel mode and just got back.

rkingsbury commented 2 months ago

@elbeejay thanks for the reminder. I am also optimistic that I can finish within the next 3 weeks; planning to get started toward the end of this week. At worst I'd anticipate I might as for 1 additional week, but if I think I'll need to do so, I'll let you know here.

elbeejay commented 2 months ago

Fantastic @BenWinchester @yalinli2 @rkingsbury - thank you all for your updates! We sincerely appreciate it.

yalinli2 commented 2 months ago

@elbeejay Just added my comments, overall a nice contribution. I noted some minor issues that would be good to fix prior to acceptance, thanks!

elbeejay commented 2 months ago

@yalinli2 that looks great - @rodoulak please refer to the very minor comments in @yalinli2's reviewer checklist above.

rodoulak commented 2 months ago

Review checklist for @yalinli2

Conflict of interest

  • [x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

General checks

Source codes available.

  • [x] License: Does the repository contain a plain-text LICENSE or COPYING file with the contents of an OSI approved software license?

MIT license.

  • [x] Contribution and authorship: Has the submitting author (@rodoulak) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

All commits were from rodoulak. The paper includes additional contributors.

  • [x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

The package desalsim contains simulation models for desalination and brine treatment technologies including nanofiltration/reverse osmosis, multi-effect distillation, thermal crystallization, chemical precipitation, eutectic freeze crystallization, and electrodialysis with membrane.

Though the mathematical development of these models have been described elsewhere, the Python implementation would be useful to researchers.

  • [x] Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.

The paper contains no original data.

  • [x] Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.

The paper contains no original results (there are tutorials, examples, and tests included in the package, see comments below on them).

The paper contains no data on humans subjects or animals.

Functionality

  • [x] Installation: Does installation proceed as outlined in the documentation?

pip install desalsim worked, but pip install -r requirements.txt didn't, requirements.txt file missing.

  • [x] Functionality: Have the functional claims of the software been confirmed?

The examples and tutorials largely worked, but some of them need to be updated. E.g., RuntimeWarning triggered for the default values in example 1: image

In some tutorials, Desalsim is used instead of desalsim, led to some importing errors.

Other minor errors in the tutorials that prevent new users to directly run them, e.g.,

Qf_nf was not defined in

https://github.com/rodoulak/desalsim/blob/8a2dc8dbf7d46574e23c3cf97259fa61c2d483e7/example/Example_1_Tutorial.md?plain=1#L118

Should this be just Qf?

I'm not sure if there are packages that can test .md files, but if the tutorials are in Jupyter Notebook, we can use automated tests.

  • [x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

There are no performance claims of the software.

Documentation

  • [x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?

Motivation/purpose statement included in README and the paper.

  • [x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.

Installation instructions are provided, though in some places Desalsim should be replaced with desalsim.

  • [x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).

Examples included, but the authors are recommended to double-check the tutorials to make sure they've been updated throughout. Would be good to add automated tests.

  • [x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?

A separate readthedocs page is available.

  • [x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?

Test files available, but recommend renaming the scripts (e.g., start with test_) so that they can be auto-detected by pytest. Also, one of tests failed: image

  • [x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Short description on how to contribute included in README.

Software paper

  • [x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
  • [x] A statement of need: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
  • [x] State of the field: Do the authors describe how this software compares to other commonly-used packages?

The paper indicated a general lack of OSS for simulation of desalination and minerals recovery systems. No comparison was provided.

  • [x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?

The paper was well written. Only a possible misspelling (valisated should probably be validated). Also maybe want to capitalize the "resource" in the tags.

There are some issues in the mathematical description:

  • No Table B.1 was provided.
  • Extra blank space in the first paragraph of A.6. for EDBM model.

Please note that I didn't check the actual equations - I assume they and their implementation in the package are all correct. I'm not a desal person, so I'm not aware if there are good benchmark models that can be used to test the accurate of certain models. Maybe other reviewers have insight.

  • [x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

A separate bibliography file is included, looks good as in the proof.

Comments to be addressed:

  • [x] Add requirements.txt.
  • [x] Fix minor bugs in tutorials/examples.
  • [x] Automate test, can consider adding a GitHub workflow.
  • [x] Fix the issues in mathematical description.
rodoulak commented 2 months ago

@yalinli2 thank you for your comments. @elbeejay I addressed all the comments from @yalinli2

yalinli2 commented 2 months ago

Hi @rodoulak , just one minor thing in the test, I think the expected_solid_mass here is off by 1000, probably shouldn't include the 1000 factor in the calculation, thanks! https://github.com/rodoulak/desalsim/blob/e6389ba87bfa2ed9b9152c4dc1bc7ce2fb579b86/tests/test_thermal_cryst.py#L38

rodoulak commented 2 months ago

Hi @yalinli2, Thank you for your comment.

I updated the line where the solution density was calculated because the units were incorrect. Previously, the density was in kg/m³ instead of kg/L, which is why the tests/test_thermal_cryst.py tests were off by a factor of 1000.

Here is the revised line: tests/test_thermal_cryst.py#L21.

Thanks again for your attention to detail!

yalinli2 commented 2 months ago

@rodoulak looks great, thanks!

rkingsbury commented 2 months ago

@elbeejay thanks for the reminder. I am also optimistic that I can finish within the next 3 weeks; planning to get started toward the end of this week. At worst I'd anticipate I might as for 1 additional week, but if I think I'll need to do so, I'll let you know here.

Hi @elbeejay just to follow up on the above - I think I will need until at least the end of this week, possibly into next week, to finish this review. Apologies that I haven't been able to move forward more quickly, but I haven't forgotten!

rkingsbury commented 2 months ago

@rodoulak I have begun my review and left some comments in my checklist concerning the "Software Paper" section. Please see above and let me know your thoughts. I haven't dug much into the software itself yet, but will do so soon.

elbeejay commented 2 months ago

No problem, we appreciate you staying in touch and keeping us updated @rkingsbury

elbeejay commented 2 months ago

@BenWinchester checking in now that it has been a few weeks, do you anticipate being able to conduct your review soon?

BenWinchester commented 2 months ago

@elbeejay , sorry for the delay in getting back to you! I've now gone through the checklist and have comments below :smiley:. In general, the code is in good shape, but there are quite a few errors in the examples which, ideally, should all be fixed before the software comes back for a second round of reviewing: I've done my best to highlight errors in the example scripts, but I likely haven't been exhaustive (and was unable to complete the examples as the number of undefined variables meant that I could no longer guess what variables should be implemented) and have missed a few, so I recommend that the authors go through the documentation thoroughly and check for any and all errors :smile:.

I also encountered some deprecation warnings in the code, which it would be nice to have cleared up:

example\efc_unit_f.py:469: DeprecationWarning: Conversion of an array with ndim > 0 to a scalar is deprecated, and will error in future.

There were several caused by this example, on lines 432, 469, 478, , 486, 523, 532, and 540.

With regards the paper, I thought it was written well. The only comment I have is in response to authoship:

Does the full list of paper authors seem appropriate and complete?

For authorship on JOSS papers, general membership of, or adminstration of, a research group isn't sufficient to be an author. As only the submitting author appears to have contributed to the repository, it's unclear what role(s) these other authors played. However, if the submitting author is happy that everyone listed on the paper was actively involved in the project, then I think this is fine; I just thought I'd note it here, but I don't have an issue with the list :smiley:

rodoulak commented 2 months ago

@rkingsbury, thank you for your comments. @elbeejay, I addressed all the comments from @rkingsbury on the paper.

rodoulak commented 2 months ago

@BenWinchester, thank you for your comments. @elbeejay, I addressed all the comments except the one on the __init__.py file. Regarding the __init__.py file, please note that the package is named desalsim and should be imported using import desalsim, rather than import Desalsim. I’m happy to discuss any further questions or concerns.

BenWinchester commented 2 months ago

@rodoulak , thanks for addressing, will take a look through again and check off the ones that are done :smiley:

For __init__.py, I'm not sure what you mean by

should be imported using desalsim rather than import Desalsim

Is your __init__.py file to expose APIs that are to be accessed externally, i.e., when the package is installed? Currently, it doesn't seem to be being used as such: you can import modules that aren't explicitly stated in __init__.py when you install the package which is available on pip and, if you build the .whl (wheel) from source and install, the package fails during the __init__.py file because __init__.py appears to be wanting to import from the installed package rather than exposing the methods/classes/modules from the repository.

I hope the above makes sense? You can take a look at the documentation for the __init__.py files to see why this error is occurring.

(Will take a look through the examples to check that they now run :smiley: )

BenWinchester commented 2 months ago

@BenWinchester, thank you for your comments. @elbeejay, I addressed all the comments except the one on the __init__.py file. Regarding the __init__.py file, please note that the package is named desalsim and should be imported using import desalsim, rather than import Desalsim. I’m happy to discuss any further questions or concerns.

@rodoulak , I started going through the examples again and they're failing this time for a different reason. E.G., in Example 1, the code imports desalsim.constants,

from desalsim.density_calc import density_calc
import desalsim.constants
import desalsim.scaleup

and then tries to access variables from the constants module

# Molecular weights 
MW_Na=constants.MW_Na
MW_Cl=constants.MW_cl
MW_SO4=constants.MW_so4
MW_K=constants.MW_K
MW_Ca=constants.MW_Ca
MW_Mg=constants.MW_Mg
MW = [MW_Na, MW_Cl, MW_K, MW_Mg, MW_Ca, MW_SO4]

However, the issue is that the import doesn't utilise the as functionality within Python:

>>> import desalsim.constants
>>> [entry for entry in locals() if "constants" in entry]
[]
>>> # There is no entry called `constants` available in the namespace
>>> import desalsim.constants  as constants
>>> [entry for entry in locals() if "constants" in entry]
['constants']
>>> # Using `as` in the import function call allows for the code to work

There are also other errors down the file, e.g., in 3.2.2 the code tries to use the variable Mc:

    # Feed flow rate L/h
Q_in_edbm=M_mfpfr_out+Mc # Where M_mfpfr_out is the effluent from MF-PFR and Mc the effluent from ED

However, this variable isn't defined or imported and so isn't in the scope.

There are also now errors occurring (typos) within the latest (1.0.9.1) update:

>>> import example_1
...
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\bened\PhD Synced\Reviewing\desalsim\example\example_1.py", line 774, in <module>
    eq_c[i]=scaleup.scaleup_eq(eq_c[i],Mf_basic_sc[i],Mf_sce[i],tec_names[i])
            ^^^^^^^^^^^^^^^^^^
AttributeError: module 'desalsim.scaleup' has no attribute 'scaleup_eq'. Did you mean: 'scaleup'?

Please go through both example markdown files and correct all issues, i.e., check that the instructions work from start-to-end 😄 (I'm happy to keep checking for errors like this, but I think it will cause a lot of churn on this review thread!)

Please let me know if anything is unclear 😄

rodoulak commented 2 months ago

Hi @BenWinchester,

Thank you very much for taking the time to go through the examples again.

Regarding the first point about the constants module and the scaleup (see There are also now errors occurring (typos) within the latest (1.0.9.1) update:), I have updated the imports to: 

from desalsim import constants

and 

from desalsim import scaleup

which should resolve the issue with accessing the variables. I’ve tested these changes, and they are now functioning correctly.

For the second point concerning the variable Mc in section 3.2.2, I realize that this may have caused some confusion. The example tutorial is designed to demonstrate how users can integrate different technologies and analyze results, but it does not walk through every step in detail for all the technologies included in Example 1. Specifically, it focuses on the two first units (nanofiltration and MF-PFR) and explains the general approach to calculating the concentration of a mixed stream. The complete steps for the other two technologies in Example 1 are detailed in their respective tutorials. However, I understand that this might not have been entirely clear, so I will make it more explicit in the documentation to avoid any further confusion.

Additionally, I reviewed both example markdown files to ensure all instructions work seamlessly from start to finish and correct any errors or typos that may have occurred in the latest update.

Please don’t hesitate to reach out if you have any further questions or suggestions 😄.

BenWinchester commented 2 months ago

@rodoulak , thanks for getting back so quickly! 😄 I'll take a look through the examples and see whether they're all working.

For the bug in example_1.py for version 1.0.9.1, did you push a pypi fix btw?

There's also the comment re: functionality documentation where the exposed APIs aren't documented. Let me know when you've addressed it and I'll take a look at the code again.

Also, there's still the __init__.py error which means that the code can't be installed from a version downloaded from Github (as is suggested in the documentation) as the .whl file that builds has errors.

Let me know when you've fixed the above and I'll take a look through again: it's just the installation, functionality and functionality-documentation checkboxes to cross out above 😃

rkingsbury commented 2 months ago

@rkingsbury, thank you for your comments. @elbeejay, I addressed all the comments from @rkingsbury on the paper.

Thank you @rodoulak . I'll take a look.

@editorialbot generate pdf

rkingsbury commented 2 months ago

@editorialbot generate pdf

editorialbot commented 2 months ago

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

rkingsbury commented 2 months ago

Hi @rodoulak , see additional comments regarding the references in the revised pdf:

The list of references is complete. However, there appear to be many references in the Ref List which are not cited in the paper. Some of these appear to be cited in the Mathematical description .pdf, but that file has its own reference list.

In the revised .pdf I still see only 9 citations in the paper but 19 references in the reference list. Unless the journal policy is that all SI reference should also be included in the main text, anything that is not cited in the paper should be removed. (Maybe @elbeejay can comment?)

Also can you include a DOI for the authorea preprint reference?

Related - Thanks to @yalinli2 's comments I see that there is an additional pdf called Mathematical Description, but I do not see this pdf referenced anywhere in the main paper or in the documentation. What is it's intended purpose?

Thank you for adding an explicit reference to the Mathematical Description. To minimize ambiguity, can you clean up the link in the revised pdf to use the same term for the file and the supporting information? E.g., just link the text "Supporting Information" and rename that file to "Supporting Information", or revise the text to say "are provided in the Mathematical Description"?

image

rkingsbury commented 2 months ago

Related, line 36: I feel your statement of need should mention the U.S. Dept. of Energy's WaterTAP, which is a significant, recent, open-access effort to develop a process simulation platform for water desalination and brine treatment. I think your contribution is valuable and distinct, but it would be helpful to the "average" reader for you to briefly (1 sentence) explain why they might choose to use DesalSim and not WaterTAP. (For example, different model coverage, simpler to learn, etc.)

@rodoulak thank you for including a reference to WaterTAP. To more clearly differentiate your software's capabilities, please revise this sentence slightly (because WaterTAP does include unit models for RO, NF, and some crystallization)

While it provides valuable simulation capabilities, it mainly focuses on desalination technologies and lacks extensive several important brine treatment technologies such as chemical precipitation and crystallization.

In addition, either in the sentence above or in the section below, I suggest you add a statement that desalsim is perhaps intended for a slightly different audience. WaterTAP is very sophisticated and powerful but has a steep learning curve and requires considerable knowledge of python and numerical solvers. desalsim, on the other hand, appears much simpler for a non-expert researcher to set up and run.

there is a need for a unified tool. Our software addresses this need by integrating a diverse range of technologies—reverse osmosis, nanofiltration, multi-effect distillation, chemical precipitation, eutectic freeze crystallization, electrodialysis, and thermal crystallization—into a comprehensive platform. This platform not only models these processes but also provides detailed techno-economic and environmental analyses.

I am only placing such emphasis on this aspect because I can tell you, at least in the U.S., that WaterTAP has gotten a lot of attention in the desalination community in recent years, and I think in order for desalsim to reach the audience and have the impact that you intend, it is important in your paper to be as clear as you can about what sets it apart / why someone might want to use it instead of WaterTAP (since at first glance, they appear to be trying to achieve similar goals).

Finally, in the reference the authors are listed as "contributors, W." which is a bit awkward. Can you change to perhaps U.S. Dept. of Energy?

rkingsbury commented 2 months ago

I will hold off on reviewing the functionality until you address some of the concerns @BenWinchester raised.

rodoulak commented 2 months ago

Hi @rkingsbury,

Thank you again for your comments!

I have revised the paper accordingly:

  1. Reference List Issue: I reviewed the reference list and confirmed that all references are cited in the main text. However, there seems to be an issue when converting the file to PDF. For example:

"However, in the desalination field, open-access simulation tools are notably lacking. While commercial software programs, like WAVE [@dupontwebsite], exist for membranes, and numerous publications discuss techno-economic models for desalination [@el2002fundamentals] and brine treatment technologies [@xevgenos2015design; @micari2020techno; @chen2021zero; @panagopoulos2021beneficiation; @poirier2022techno; @morgante2022valorisation], there is a noticeable absence of open-access simulation tools in the literature. ". I replaced the commas with semicolons to separate references, but I’m not certain if this will resolve the issue in the PDF. If you have any suggestions for ensuring correct reference formatting, I would appreciate your guidance.

  1. Supplementary Information: To avoid any confusion, I have renamed the "Supplementary Information" to "Mathematical Description" document.
  2. Software Capabilities: I revised the manuscript text to reflect your suggestions regarding the software’s capabilities and differentiation from other tools like WaterTAP.
  3. WaterTAP Citation: The author attribution for WaterTAP has been updated as per your recommendation.
elbeejay commented 2 months ago

Hi @rodoulak , see additional comments regarding the references in the revised pdf:

The list of references is complete. However, there appear to be many references in the Ref List which are not cited in the paper. Some of these appear to be cited in the Mathematical description .pdf, but that file has its own reference list.

In the revised .pdf I still see only 9 citations in the paper but 19 references in the reference list. Unless the journal policy is that all SI reference should also be included in the main text, anything that is not cited in the paper should be removed. (Maybe @elbeejay can comment?)

Thanks for flagging this @rkingsbury, you are correct, we only want references in the paper itself to be included in the paper's reference list. I agree with the comment regarding the proposed change to the "supporting information" text as well.

rodoulak commented 2 months ago

Hi @BenWinchester,

Thank you for your detailed feedback. I’ve reviewed the issues related to the __init__.py file and made the following updates:

1. Relative Imports: I’ve replaced the absolute imports with relative imports in the init.py file (according to the documentation) to ensure that the modules are correctly imported from the local repository, even when the package is installed directly from GitHub.

2. Installation Testing: I’ve tested the package installation from source and verified that it works correctly without errors.

3. Clarification on the Purpose of __init__.py (see comment): The primary purpose of the __init__.py file in our package is indeed to expose key functions and classes that users will need to access when they import the package. We have structured the file to ensure that these APIs are readily available and easy to use.

4. Regarding typo errors in the latest version ( see comment): The errors have been resolves, the scaleup function has been updated in the examples (see line 232 in example_1.py,line 290 in example_1.py, and line 274 in example_1.py.

Please let me know if there are any further issues with the new __init__.py or if you have any further questions or comments.

elbeejay commented 2 months ago

Hello @rodoulak, @yalinli2, @BenWinchester, and @rkingsbury - I wanted to ask how things are going.

@rodoulak have you finished making revisions and addressing reviewer comments?

@yalinli2, @BenWinchester, and @rkingsbury are you all ready to take another look?

Thanks all for participating in this process!

rodoulak commented 2 months ago

Hi @elbeejay,

I finished with revisions and addressed reviewer comments.

I have only one question regarding the references, as I also mentioned in a comment above: Reference List Issue: I reviewed the reference list and confirmed that all references are cited in the main text. However, there seems to be an issue when converting the file to PDF. For example: "However, in the desalination field, open-access simulation tools are notably lacking. While commercial software programs, like WAVE [@dupontwebsite], exist for membranes, and numerous publications discuss techno-economic models for desalination [@el2002fundamentals] and brine treatment technologies [@xevgenos2015design; @micari2020techno; @chen2021zero; @panagopoulos2021beneficiation; @poirier2022techno; @morgante2022valorisation], there is a noticeable absence of open-access simulation tools in the literature. "

I replaced the commas with semicolons to separate references, but I’m not certain if this will resolve the issue in the PDF. If you have any suggestions for ensuring correct reference formatting, I would appreciate your guidance.

BenWinchester commented 2 months ago

@rodoulak , thanks for giving a go at addressing the comments. Just a few things still need fixing on my end:

elbeejay commented 1 month ago

Hi @elbeejay,

I finished with revisions and addressed reviewer comments.

I have only one question regarding the references, as I also mentioned in a comment above: Reference List Issue: I reviewed the reference list and confirmed that all references are cited in the main text. However, there seems to be an issue when converting the file to PDF. For example: "However, in the desalination field, open-access simulation tools are notably lacking. While commercial software programs, like WAVE [@dupontwebsite], exist for membranes, and numerous publications discuss techno-economic models for desalination [@el2002fundamentals] and brine treatment technologies [@xevgenos2015design; @micari2020techno; @chen2021zero; @panagopoulos2021beneficiation; @poirier2022techno; @morgante2022valorisation], there is a noticeable absence of open-access simulation tools in the literature. "

I replaced the commas with semicolons to separate references, but I’m not certain if this will resolve the issue in the PDF. If you have any suggestions for ensuring correct reference formatting, I would appreciate your guidance.

To group multiple citations the semi-colon should work just fine.

rodoulak commented 1 month ago

Ηι @BenWinchester,

Thank you again for your feedback. I have made several updates to address your comments:

  • [x] For the functionality documentation:

    • density_calc, HClAddition, energycons, ElectrodialysisCalc, and econom are still missing docstrings. For density_calc, for the docstring to be properly accessible when the API is used, the docstring should be a functional dosctring rather than a module docstring;

Docstrings have been added to density_calc, HClAddition, energycons, ElectrodialysisCalc, and econom, ensuring clarity on functionality. The density_calc docstring is now a proper functional docstring as requested.

  • [x] As per comments above, the examples provided still aren't working. Currently, 3.2.2. is failling as Mc isn't defined. Please go through the examples you provide fully and ensure that there are no bugs and issues 🙂

I revised 3.2.3 Other units to clarify that detailed steps for Nanofiltration (NF) and MF-PFR are provided, while EDBM and ED steps focus on calculating mixed stream flow rates and concentrations. Links to their respective tutorials have been added, and the issue with Mc has been clarified, directing users to the ED tutorial for further steps.

The code has been revised so the above comment is clear also in the calculation of the mixed stream: 

    # Feed flow rate L/h
Q_in_edbm = M_mfpfr_out + Mc  # Where M_mfpfr_out is the effluent from MF-PFR, and Mc is the effluent from ED
# Mc is calculated in the ED tutorial, refer to ED Tutorial for detailed steps on calculating Mc (see https://github.com/rodoulak/desalsim/blob/main/Tutorials/ED_Tutorial.md)

The example code in example_1.py works properly.

rkingsbury commented 1 month ago

@elbeejay it looks like significant revisions have been made; I should be able to take another look next week.

elbeejay commented 1 month ago

Awesome, appreciate the open communication @rkingsbury

elbeejay commented 1 month ago

@BenWinchester if you could take a look at @rodoulak's comment above when you get a chance, it looks like your last few comments may be addressed.