joss-reviews icon indicating copy to clipboard operation
joss-reviews copied to clipboard

[REVIEW]: ParMOO: A Python library for parallel multiobjective simulation optimization

Open editorialbot opened this issue 2 years ago • 24 comments

Submitting author: @thchang (Tyler Chang) Repository: https://github.com/parmoo/parmoo Branch with paper.md (empty if default branch): joss Version: v0.1.0 Editor: @kellyrowland Reviewers: @Viech, @jdreo, @jonjoncardoso Archive: Pending

Status

status

Status badge code:

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

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

@Viech & @jdreo, 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 @kellyrowland 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 @Viech

📝 Checklist for @jonjoncardoso

editorialbot avatar Jun 10 '22 16:06 editorialbot

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 avatar Jun 10 '22 16:06 editorialbot

Software report:

github.com/AlDanial/cloc v 1.88  T=0.16 s (648.5 files/s, 125611.4 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          60           2591           4937           9191
reStructuredText                33            807            887           1201
SVG                              3              1              2            196
Markdown                         1             22              0            115
TeX                              1              7              0             88
Bourne Shell                     1             15              6             87
YAML                             3             16              6             74
make                             2             13              7             33
DOS Batch                        1              8              1             26
-------------------------------------------------------------------------------
SUM:                           105           3480           5846          11011
-------------------------------------------------------------------------------


gitinspector failed to run statistical information for the repository

editorialbot avatar Jun 10 '22 16:06 editorialbot

Wordcount for paper.md is 800

editorialbot avatar Jun 10 '22 16:06 editorialbot

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- None

MISSING DOIs

- 10.1109/access.2020.2990567 may be a valid DOI for title: pymoo: Multi-Objective Optimization in Python
- 10.1007/s10589-017-9955-0 may be a valid DOI for title: A multi-objective DIRECT algorithm for ship hull optimization
- 10.1145/3529258 may be a valid DOI for title: Algorithm XXXX: VTMOP: Solver for Blackbox Multiobjective Optimization Problems
- 10.1287/ijoc.2019.0902 may be a valid DOI for title: PyMOSO: Software for Multi-Objective Simulation Optimization with R-PERLE and R-MinRLE
- 10.1038/s41586-021-03213-y may be a valid DOI for title: Bayesian reaction optimization as a tool for chemical synthesis
- 10.1109/tpds.2021.3082815 may be a valid DOI for title: libEnsemble: A Library to Coordinate the Concurrent Evaluation of Dynamic Ensembles of Calculations

INVALID DOIs

- None

editorialbot avatar Jun 10 '22 16:06 editorialbot

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

editorialbot avatar Jun 10 '22 16:06 editorialbot

My calendar is full until the 23rd but I will aim to review this month.

Viech avatar Jun 10 '22 17:06 Viech

Thanks for the heads up, noted.

kellyrowland avatar Jun 10 '22 17:06 kellyrowland

I'd also be happy to review this one if more reviewers are required

jonjoncardoso avatar Jun 20 '22 14:06 jonjoncardoso

Review checklist for @Viech

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

  • [x] Repository: Is the source code for this software available at the https://github.com/parmoo/parmoo?
  • [x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • [x] Contribution and authorship: Has the submitting author (@thchang) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
  • [x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

  • [x] Installation: Does installation proceed as outlined in the documentation?
  • [x] Functionality: Have the functional claims of the software been confirmed?
  • [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.)

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?
  • [x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • [x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • [x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • [x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
  • [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

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?
  • [x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
  • [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?

Viech avatar Jun 23 '22 15:06 Viech

Hello all,

Glad to see some reviews are coming in! Thanks all reviewers for your time!

Just wondering, will we get more detailed feedback than just this checklist? Some of these items may be self-explanatory, but for many (e.g., the License entry, statement of need, installation, functionality, and references) it is not obvious to me why we are not satisfying these requirements, and I would appreciate more detailed feedback!

Thanks, ---Tyler

P.S., I have some revisions of my own, mostly other MOO software that was published in the last couple years that I was unaware of at the time I started this project, which should be included in the JOSS paper. Also, I am working on some minor updates to the software. I am assuming that I should I wait until the first round of reviews has completed before adding any such updates to the paper, like one would for a normal journal submission?

thchang avatar Jun 29 '22 22:06 thchang

Dear Tyler,

of course I will elaborate on the checklist and give detailed feedback! I merely had to pause the review due to other obligations this week and I will continue soon.

Note that most of the non-ticked boxes either mean that I did not get to review this part yet or that I have some (minor) remark on my list concerning this point. It does not mean that I find that your submission does not meet the point.

Viech avatar Jun 30 '22 06:06 Viech

OK thanks for clarifying!

I see, sorry this is my first experience with this journal so I'm just learning how things work :) I misunderstood the checklist as your final review for the paper.

---Tyler

thchang avatar Jun 30 '22 14:06 thchang

It was a pleasure to review this! I will certainly keep this library in mind for when I need to solve a multi-objective problem with expensive evaluation.

To put my comments into perspective, I was previously aware of design of experiments type problems, various types of metaheuristics, and multi-objective optimization, but have not heard the term simulation optimization before.

I marked as nitpicks small suggestions that you are invited to ignore, and as optional suggestions larger changes that you could consider for the future but that should also not get in the way of publication. (I will tick the associated boxes when you respond to these suggestions or when all other issues are resolved.)

General checks/License

  • Nitpick: Your copyright line reads Copyright (c) 2022, parmoo. I'm not sure if mentioning the program name here is appropriate. Maybe Copyright (c) 2022, parmoo developers, or using names or institutions, would be better?

Functionality/Installation

  • Nitpick: Writing pip install [--user] parmoo[extras] might confuse inexperienced users as the [--user] is optional (and the brackets must be removed) while for [extras] the brackets are part of the input. Also, for some shells (e.g. zsh) one would need to write "parmoo[extras]" instead of parmoo[extras] (the former should work for all shells).
  • Optional suggestion: To support Windows and also Mac users, I've found Anaconda to be a useful distribution system in addition to PyPI/pip.

Documentation

  • In the Quickstart section, the concepts of design variables, simulations, objectives and constraints are well explained but the surrogate model, its scalarazation, and the acquisition function remain somewhat enigmatic. In particular, I did not initially understand why "we must add one or more acquisition functions". I suggest to either explain these concepts already in Quickstart or to refer the reader to explanations within other sections where appropriate.
  • The acquisition function is missing from the Glossary subsection of Learn about MOOPs. Non-specialist intuition could also be given in the Adding Acquisitions subsection of Write a ParMOO Script.
  • In the Minimum Working Example, it would be helpful if the output could be explained and/or plotted.
  • JOSS requirement: You should mention the target audience in the documentation.
  • JOSS requirement: You should mention how to report issues and seek support.

Software paper

  • In the summary, which I otherwise find very accessible, it is not clear to me what is meant by a "simulation-based structure". Briefly explaining this concept could make also the examples more accessible to non-specialists (here I do not understand the parts that explain where said structure comes from).
  • "Scientific software packages for solving multiobjective simulation optimization problems include": Here I would prefer to read the names of the software packages (with authors and years in parentheses). It would also be nice to have a partition into Python packages and other software (unless all of them are for Python; then this should be mentioned).
  • In the EDF example, I do not quite understand what is meant by ParMOO being used to "minimiz[e] the error between expensive simulation outputs and experimental data".
  • If available, references to the mentioned research using ParMOO would be very interesting.
  • Nitpick: The HPC abbreviation is not introduced.
  • Nitpick: For your ACM Transactions on Mathematical Software paper, in the references, maybe add a note/comment that this paper was just accepted. Otherwise, it looks like volume and issue numbers are missing.
  • Nitpick/unsure: The string "In arXiv preprint arXiv:2105.03000." in the references looks odd (but I do not know the prefered way to cite an arXiv paper in the JOSS).
  • JOSS requirement: The statement of need section should mention the target audience.
  • (In my experience, the editor will later ask you to investigate the missing DOIs that were mentioned by @editorialbot.)

Code

JOSS reviews do not seem to include code review, so all of these points are just suggestions.

  • You do not need to have a return statement at the end of a function. (Python automatically does return None at the end of a non-generator function.)
  • When using the ABC base class, you should use the abc.abstractmethod decorator instead of raising NotImplementedError(e.g. in sim_func add @abstractmethod above __call__, then inside __call__, just pass). Otherwise I do not think that adding the ABC base class has any effect.
  • You could make your xerror function future-proof by giving the arguments default values and callling it with keyword arguments, like you do with __check_optionals__ (i.e. instead of xerror(1, lb, ub, {}) you would write xerror(lb=lb, ub=ub)). This would allow you to add new checks later on.
  • Is there a reason you do from parmoo.util import xerror within __init__? It does not seem to produce a circular import if you import it at the start of the source file, which is the typical reason to import within a function. Importing at the start of the file might profit software that analyzes your code, including your IDE.
  • if 'name' in arg.keys() -> if 'name' in arg
  • You have some weird try: assert(_) except BaseException: raise _ checks in sum_sim_bound. Can you not just condition the raised exception on what you check within assert? Normally, assert is reserved for things that, when they go wrong, imply an error in the software that makes the assertion (as opposed to a user error).
  • When checking whether something is a NumPy array, the proper exception would by TypeError instead of ValueError. Although, I cannot think of a scenario where this would make any difference, so it doesn't get any more nitpicky than this!
  • In setup.py, exec(open("parmoo/version.py").read()) looks a bit dangerous. Have you considered from parmoo.version import __version__?
  • Thank you for making me aware of __slots__, I've literally never seen this before!

Issues (not impeding acceptance; only for reference)

  • https://github.com/parmoo/parmoo/issues/32

Viech avatar Jun 30 '22 19:06 Viech

hi @jdreo just checking in on this review 👋 please let me know when you think you may be able to get started or if you need to set it down.

kellyrowland avatar Jun 30 '22 21:06 kellyrowland

Thanks @Viech for your detailed review! I'll look into these issues, and we've been slowly adding a few more features on other branches, so hopefully we will roll them all into a 2nd release in the coming month(s).

I'll also wait to hear back from @jdreo before taking any actions.

@kellyrowland what is the protocol for this? Do I respond on this thread with a list of changes (similar to a regular response letter) once I've released a new version and updated the paper on the parmoo/joss branch?

thchang avatar Jul 07 '22 16:07 thchang

@thchang yep, please work on suggested edits and then let us know here once you've addressed them. I'd say to go ahead with addressing the comments posted in the issue here already (as opposed to waiting from comments from both reviewers), and then address any further edits suggested by the second reviewer.

kellyrowland avatar Jul 08 '22 19:07 kellyrowland

hi @jonjoncardoso would you still be able to and interested in reviewing this submission?

kellyrowland avatar Aug 01 '22 23:08 kellyrowland

@editorialbot generate my checklist

jonjoncardoso avatar Aug 15 '22 13:08 jonjoncardoso

@jonjoncardoso I can't do that because you are not a reviewer

editorialbot avatar Aug 15 '22 13:08 editorialbot

Ops, I thought I had already been assigned. Yes, @kellyrowland , I am happy to review this submission

jonjoncardoso avatar Aug 15 '22 13:08 jonjoncardoso

Thanks! Apologies for not catching your 👍 .

kellyrowland avatar Aug 15 '22 16:08 kellyrowland

@editorialbot add @jonjoncardoso as reviewer

kellyrowland avatar Aug 15 '22 16:08 kellyrowland

@jonjoncardoso added to the reviewers list!

editorialbot avatar Aug 15 '22 16:08 editorialbot

Review checklist for @jonjoncardoso

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

  • [x] Repository: Is the source code for this software available at the https://github.com/parmoo/parmoo?
  • [x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • [x] Contribution and authorship: Has the submitting author (@thchang) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
  • [x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

  • [ ] Installation: Does installation proceed as outlined in the documentation?
  • [ ] Functionality: Have the functional claims of the software been confirmed?
  • [ ] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • [ ] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • [ ] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • [ ] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • [ ] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • [ ] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
  • [ ] 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

Software paper

  • [ ] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
  • [ ] 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?
  • [ ] State of the field: Do the authors describe how this software compares to other commonly-used packages?
  • [ ] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
  • [ ] 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?

jonjoncardoso avatar Aug 16 '22 09:08 jonjoncardoso

Hi @openjournals/joss-eics @thchang @Viech @jonjoncardoso - I just wanted to let you all know that I will be out of office and not checking emails, Github, etc. starting September 10 and will be returning on October 3. Happy to field any questions through the end of the week.

kellyrowland avatar Sep 07 '22 17:09 kellyrowland

Thanks for the update @kellyrowland Just to update on our end as well, we are currently working on changes reflecting @Viech 's review, while merging these with the new features that we've been working on in the meantime. We should have these changes available soon, and will post a link to the PR reflecting @Viech 's suggestions when it is done.

thchang avatar Sep 07 '22 21:09 thchang

Sounds good!

kellyrowland avatar Sep 07 '22 22:09 kellyrowland

@Viech here is our response to your review. Your review was very helpful to us, thank-you for your thoroughness.

FYI, all changes to our source code and docs that you suggested are now active in the develop branch, and were addressed in the following PR, so you can conveniently view the diffs here: https://github.com/parmoo/parmoo/pull/39

Additionally, we have not yet updated the paper, but we have added a joss-rev branch, where the changes to the paper have been implemented. You can view the latest paper build here: https://github.com/parmoo/parmoo/actions/runs/3107253388 or the latest paper source here: https://github.com/parmoo/parmoo/tree/joss-rev/docs/papers/joss

Responses to your individual comments are given below:

General Comments

  • Nitpick: Your copyright line reads Copyright (c) 2022, parmoo. I'm not sure if mentioning the program name here is appropriate. Maybe Copyright (c) 2022, parmoo developers, or using names or institutions, would be better?
    • We changed the copyright to “parmoo developers”

Functionality/Installation

  • Nitpick: Writing pip install [--user] parmoo[extras] might confuse inexperienced users as the [--user] is optional (and the brackets must be removed) while for [extras] the brackets are part of the input. Also, for some shells (e.g. zsh) one would need to write "parmoo[extras]" instead of parmoo[extras] (the former should work for all shells).
    • We are now more explicit in terms of optional arguments and have added quotes. These changed the README and other doc files.
  • Optional suggestion: To support Windows and also Mac users, I've found Anaconda to be a useful distribution system in addition to PyPI/pip.
    • We have now added a parmoo recipe to conda-forge channel and updated our docs with the conda install instructions.

Documentation

  • In the Quickstart section, the concepts of design variables, simulations, objectives and constraints are well explained but the surrogate model, its scalarazation, and the acquisition function remain somewhat enigmatic. In particular, I did not initially understand why "we must add one or more acquisition functions". I suggest to either explain these concepts already in Quickstart or to refer the reader to explanations within other sections where appropriate.
    • We have now added a short summary of the framework, linked to the detailed documentation within the README/quickstart.
  • The acquisition function is missing from the Glossary subsection of Learn about MOOPs. Non-specialist intuition could also be given in the Adding Acquisitions subsection of Write a ParMOO Script.
    • Thanks for catching this, we have now added this.
  • In the Minimum Working Example, it would be helpful if the output could be explained and/or plotted.
    • We now include a brief description of the results variable, and provide code for plotting a scatterplot of the results, showing the nondominated points. This has been updated in the README, quickstart section of the docs, and the basic tutorials.
  • JOSS requirement: You should mention the target audience in the documentation.
    • We have added a paragraph about our intended audience and usage to the landing page of the readthedocs site.
  • JOSS requirement: You should mention how to report issues and seek support.
    • In the resources in documentation we now explicitly say that one can report issues or seek support via github or the provided email.

Software paper

  • In the summary, which I otherwise find very accessible, it is not clear to me what is meant by a "simulation-based structure". Briefly explaining this concept could make also the examples more accessible to non-specialists (here I do not understand the parts that explain where said structure comes from).
    • We have reworded how this property is introduced in the summary. Additionally, we have added a methodology section, which both summarizes how ParMOO works and gives further details on how the simulation structure is exploited. Finally, by clarifying the EDF example (below), we hope that we have further improved clarity through the example.
  • "Scientific software packages for solving multiobjective simulation optimization problems include": Here I would prefer to read the names of the software packages (with authors and years in parentheses). It would also be nice to have a partition into Python packages and other software (unless all of them are for Python; then this should be mentioned).
    • We have added several additional packages to the list that were omitted in the initial submission, added package names, and clarified which packages were written in Python.
  • In the EDF example, I do not quite understand what is meant by ParMOO being used to "minimiz[e] the error between expensive simulation outputs and experimental data".
    • We have updated the description of the EDF calibration and chemical synthesis problems to explicitly specify how the problem is given to ParMOO, and what simulation-based structure is exploitable in each of these contexts. In the case of the EDF calibration, we also added a reference to the single-objective software POUNDERS, which attempts to exploit a similar structure in single-objective problems.
  • If available, references to the mentioned research using ParMOO would be very interesting.
    • N/A – Unfortunately, we do not have any references for these applications at this time.
  • Nitpick: The HPC abbreviation is not introduced.
    • We have now defined the abbreviation before its first usage.
  • Nitpick: For your ACM Transactions on Mathematical Software paper, in the references, maybe add a note/comment that this paper was just accepted. Otherwise, it looks like volume and issue numbers are missing.
    • The TOMS paper has appeared in print, so we have updated the reference accordingly.
  • Nitpick/unsure: The string "In arXiv preprint arXiv:2105.03000." in the references looks odd (but I do not know the prefered way to cite an arXiv paper in the JOSS).
    • This preprint has now been moved to the “Just Online” section of Numerical Algorithms, so we have changed the reference anyway.
  • JOSS requirement: The statement of need section should mention the target audience.
    • We have added a statement on the target audience (same as in the docs) to the bottom of the statement of need section.
  • (In my experience, the editor will later ask you to investigate the missing DOIs that were mentioned by @editorialbot.)
    • We have now added either DOIs or URLs (when no DOI is available) to all refs.

Source Code:

  • You do not need to have a return statement at the end of a function. (Python automatically does return None at the end of a non-generator function.)
    • Your comment is well taken but we prefer to explicitly include the return statements for clarity reasons. In our experience, this helps with maintainability, especially with complicated functions containing multiple return statements, by explicitly marking the possible return locations
  • When using the ABC base class, you should use the abc.abstractmethod decorator instead of raising NotImplementedError(e.g. in sim_func add @abstractmethod above call, then inside call, just pass). Otherwise I do not think that adding the ABC base class has any effect.
    • Thanks for the tip, we weren’t sure if we were using the ABC library right, so that makes sense now. There are a few methods in each class that either (1) have a default implementation or (2) need not be implemented by the child class, depending on the solver techniques being used. To allow for this, we left the @abstractmethod tag off of these methods, and left the raise NotImplementedError in the body of these optional methods. This way, if a user tries to use a method that does not implement one of these optional methods, with another solver that requires the implementation of these optionals methods, then the mis-match will be detectable. For the majority of methods in each class, however, we have added the decorator, as you suggested and updated our test suite accordingly.
  • You could make your xerror function future-proof by giving the arguments default values and callling it with keyword arguments, like you do with check_optionals (i.e. instead of xerror(1, lb, ub, {}) you would write xerror(lb=lb, ub=ub)). This would allow you to add new checks later on.
    • Good tip! We have done as suggested.
  • Is there a reason you do from parmoo.util import xerror within init? It does not seem to produce a circular import if you import it at the start of the source file, which is the typical reason to import within a function. Importing at the start of the file might profit software that analyzes your code, including your IDE.
    • Fixed. As you know, there are several reasons to avoid unnecessary imports, so we had wanted to do so by placing imports that are not used throughout the containing class inside the individual methods that actually use them. This was probably an oversight, however, as in the case of init, everything inside the init function will alway be imported if the class is to be used, and as you pointed out, there is also no risk of circular import.
  • if 'name' in arg.keys() -> if 'name' in arg
    • Thanks for the tip! We have replaced all occurrences as suggested.
  • You have some weird try: assert(_) except BaseException: raise _ checks in sum_sim_bound. Can you not just condition the raised exception on what you check within assert? Normally, assert is reserved for things that, when they go wrong, imply an error in the software that makes the assertion (as opposed to a user error).
    • Fixed. We were doing it this way for a while to reduce the number of lines of code and avoid having a double-check (first, does the initialization succeed, then is the result of the correct type). We thought we had corrected all occurrences, but missed a couple.
  • When checking whether something is a NumPy array, the proper exception would by TypeError instead of ValueError. Although, I cannot think of a scenario where this would make any difference, so it doesn't get any more nitpicky than this!
    • Good catch. We made this mistake early-on in the development process, and tried to go back and fix it later, but it looks like we missed many occurrences of this mistake. We went back and think that we have corrected all instances now (including several instances where we were using a ValueError when checking the Python intrinsic types, and when checking whether an input is callable).
  • In setup.py, exec(open("parmoo/version.py").read()) looks a bit dangerous. Have you considered from parmoo.version import version?
    • setup.py is used to create the package. Importing from the package while creating the package seems dangerous, and other projects do it this way. Open to feedback if you have further insight.
  • Thank you for making me aware of slots, I've literally never seen this before!
    • Great! As you’ve probably read, it allegedly gives significant speedup in certain situations.

Issues (not impeding acceptance; only for reference)

thchang avatar Sep 22 '22 17:09 thchang

I've re-read the paper, which to me is much clearer now! I've also reviewed parmoo/parmoo#39 and this looks good to me. From my point of view, the submission can be accepted as is.

  • In setup.py, exec(open("parmoo/version.py").read()) looks a bit dangerous. Have you considered from parmoo.version import version?
  • setup.py is used to create the package. Importing from the package while creating the package seems dangerous, and other projects do it this way. Open to feedback if you have further insight.

Unfortunately I do not have any more insight and I suppose exec is not so different from import, anyway.

In our experience, this helps with maintainability, especially with complicated functions containing multiple return statements, by explicitly marking the possible return locations

Just as a side remark, if this kind of explicit/self-documenting code is important to you, I should advertise Python's new type hinting features! Type hints allow you to document function argument and return types and to have those statically checked. You can write, for instance, def divide(n: int, d: int) -> float: and your IDE/CI can tell you when this function is not called with integer arguments or when it does not always return a float. However some care must be taken with respect to which Python versions you support as these features were incrementally added since version 3.5.

Viech avatar Sep 25 '22 11:09 Viech

Thanks @Viech

We are aware of the hinting features, and it is definitely something that we want to look into in the future, but I think it is not so easy in our case since we are simulating polymorphism by defining some function behaviors based on the input types (thereby allowing for multiple different input types).

Not sure what the recommendation/policy is in these cases: e.g., suppose arg1 could be an int, list, or dict type, and the output will be a float, str, or None depending on what the input type is.

In any case, we'll look into it in the future, at least for the simpler function definitions. Thanks for all your feedback so far!

thchang avatar Oct 03 '22 22:10 thchang