openjournals / joss-reviews

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

[REVIEW]: BoxKit: A Python library to manage analysis of block-structured simulation datasets #5649

Closed editorialbot closed 9 months ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@akashdhruv<!--end-author-handle-- (Akash Dhruv) Repository: https://github.com/akashdhruv/BoxKit Branch with paper.md (empty if default branch): development Version: 2023.12 Editor: !--editor-->@kellyrowland<!--end-editor-- Reviewers: @rvg296, @Abinashbunty Archive: 10.5281/zenodo.10257565

Status

status

Status badge code:

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

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

@rvg296 & @AnnikaStein, 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 @rvg296

πŸ“ Checklist for @Abinashbunty

editorialbot commented 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
editorialbot commented 1 year ago
Software report:

github.com/AlDanial/cloc v 1.88  T=0.18 s (903.6 files/s, 177908.3 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
JavaScript                      13           2405           2470           9222
HTML                            31            605             93           4863
C/C++ Header                    18            478            257           2719
Python                          51            930            846           2699
SVG                              3              0              0           2673
CSS                              4            191             35            759
YAML                             7             22             29            227
reStructuredText                28            213            380            225
C++                              6             28             50            180
Markdown                         1             22              0            106
TeX                              1              5              0             68
DOS Batch                        1              8              1             26
make                             3             10             10             26
-------------------------------------------------------------------------------
SUM:                           167           4917           4171          23793
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 1 year ago

Wordcount for paper.md is 840

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

OK DOIs

- 10.1016/j.softx.2022.101168 is OK
- 10.5281/zenodo.8039787 is OK
- 10.1088/0067-0049/192/1/9 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 1 year ago

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

AnnikaStein commented 1 year ago

Review checklist for @AnnikaStein

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

akashdhruv commented 1 year ago

@editorialbot check references

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

OK DOIs

- 10.1016/j.softx.2022.101168 is OK
- 10.5281/zenodo.8039787 is OK
- 10.1088/0067-0049/192/1/9 is OK

MISSING DOIs

- None

INVALID DOIs

- None
akashdhruv commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

rvg296 commented 1 year ago

@editorialbot generate my checklist

editorialbot commented 1 year ago

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@editorialbot commands

akashdhruv commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

akashdhruv commented 1 year ago

@rvg296 I believe the command is:

@editorialbot generate my checklist

rvg296 commented 1 year ago

@rvg296 I believe the command is:

@editorialbot generate my checklist

Yes I have corrected it

akashdhruv commented 1 year ago

@editorialbot check references

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

OK DOIs

- 10.1016/j.softx.2022.101168 is OK
- 10.5281/zenodo.8039787 is OK
- 10.1088/0067-0049/192/1/9 is OK

MISSING DOIs

- None

INVALID DOIs

- None
akashdhruv commented 1 year ago

@editorialbot generate pdf

@rvg296 @AnnikaStein Please use this version here for review: I have included reference to performance data repository.

editorialbot commented 1 year ago

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

akashdhruv commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

AnnikaStein commented 1 year ago

Review checklist for @AnnikaStein

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

rvg296 commented 1 year ago

Review checklist for @rvg296

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

kellyrowland commented 1 year ago

hi @AnnikaStein @rvg296 thanks for getting started here - please let me know if you have any questions or concerns.

kellyrowland commented 1 year ago

hi @AnnikaStein @rvg296 friendly reminder about this review πŸ‘‹ please let me know if you have any questions or concerns, or if you need to set this down.

rvg296 commented 1 year ago

Hi @kellyrowland dont have any questions at this moment. In the process of review currently stage by stage.

AnnikaStein commented 1 year ago

Same for me, I also donβ€˜t have any questions at the moment, but just recently came back from a vacation. Continuing reviewing this + next week.

kellyrowland commented 1 year ago

Great, thank you both!

rvg296 commented 1 year ago

@akashdhruv May be a minor edit, but in line 54 of the paper should'nt the Scikit method be named as skimage.measure instead of skimage_measurefor measuring the bubble shape and size??

akashdhruv commented 1 year ago

@akashdhruv May be a minor edit, but in line 54 of the paper should'nt the Scikit method be named as skimage.measure instead of skimage_measurefor measuring the bubble shape and size??

I will address this.

akashdhruv commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

akashdhruv commented 1 year ago

@akashdhruv May be a minor edit, but in line 54 of the paper should'nt the Scikit method be named as skimage.measure instead of skimage_measurefor measuring the bubble shape and size??

I will address this.

Done!

akashdhruv commented 11 months ago

@kellyrowland @rvg296 @AnnikaStein

I transferred the repository to: https://github.com/Box-Tools/BoxKit to meet licensing requirements from Argonne National Laboratory. It should not change anything.

akashdhruv commented 11 months ago

@editorialbot generate pdf

editorialbot commented 11 months ago

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

rvg296 commented 11 months ago

@akashdhruv Tried installing this in a virtual environment created with Conda with Py310. I get this installation error. (py310) C:\Users\MENDR>pip install BoxKit --user

Collecting BoxKit
  Downloading BoxKit-2023.6.7.tar.gz (852 kB)
     ---------------------------------------- 852.5/852.5 kB 1.9 MB/s eta 0:00:00
  Preparing metadata (setup.py) ... error
  error: subprocess-exited-with-error

  Γ— python setup.py egg_info did not run successfully.
  β”‚ exit code: 1
  ╰─> [10 lines of output]
      Traceback (most recent call last):
        File "<string>", line 2, in <module>
        File "<pip-setuptools-caller>", line 34, in <module>
        File "C:\Users\MENDR\AppData\Local\Temp\1\pip-install-jv7ymwcp\boxkit_f17c2e8a1aef4428bab1ef1a7d2994e5\setup.py", line 14, in <module>
          import bin.cmd as bin_cmd  # pylint: disable=wrong-import-position
        File "C:\Users\MENDR\AppData\Local\Temp\1\pip-install-jv7ymwcp\boxkit_f17c2e8a1aef4428bab1ef1a7d2994e5\bin\cmd.py", line 13, in <module>
          from cbox import cbox_build  # pylint: disable=wrong-import-position
        File "C:\Users\MENDR\AppData\Local\Temp\1\pip-install-jv7ymwcp\boxkit_f17c2e8a1aef4428bab1ef1a7d2994e5\bin\cbox.py", line 21, in <module>
          if os.path.exists(os.getenv("PWD") + "/boxkit/depends/boost")
      TypeError: unsupported operand type(s) for +: 'NoneType' and 'str'
      [end of output]

  note: This error originates from a subprocess, and is likely not a problem with pip.
error: metadata-generation-failed

Γ— Encountered error while generating package metadata.
╰─> See above for output.

note: This is an issue with the package mentioned above, not pip.
hint: See above for details.

(py310) C:\Users\MENDR>pip install BoxKit
Collecting BoxKit
  Using cached BoxKit-2023.6.7.tar.gz (852 kB)
  Preparing metadata (setup.py) ... error
  error: subprocess-exited-with-error

  Γ— python setup.py egg_info did not run successfully.
  β”‚ exit code: 1
  ╰─> [10 lines of output]
      Traceback (most recent call last):
        File "<string>", line 2, in <module>
        File "<pip-setuptools-caller>", line 34, in <module>
        File "C:\Users\MENDR\AppData\Local\Temp\1\pip-install-r2sw90yv\boxkit_decd8282603049f5bb2d36ea2947c869\setup.py", line 14, in <module>
          import bin.cmd as bin_cmd  # pylint: disable=wrong-import-position
        File "C:\Users\MENDR\AppData\Local\Temp\1\pip-install-r2sw90yv\boxkit_decd8282603049f5bb2d36ea2947c869\bin\cmd.py", line 13, in <module>
          from cbox import cbox_build  # pylint: disable=wrong-import-position
        File "C:\Users\MENDR\AppData\Local\Temp\1\pip-install-r2sw90yv\boxkit_decd8282603049f5bb2d36ea2947c869\bin\cbox.py", line 21, in <module>
          if os.path.exists(os.getenv("PWD") + "/boxkit/depends/boost")
      TypeError: unsupported operand type(s) for +: 'NoneType' and 'str'
      [end of output]

  note: This error originates from a subprocess, and is likely not a problem with pip.
error: metadata-generation-failed

Γ— Encountered error while generating package metadata.
╰─> See above for output.

note: This is an issue with the package mentioned above, not pip.
hint: See above for details.

Are there any specific dependencies that needs to be installed prior to this?

akashdhruv commented 11 months ago

@akashdhruv Tried installing this in a virtual environment created with Conda with Py310. I get this installation error. (py310) C:\Users\MENDR>pip install BoxKit --user

Collecting BoxKit
  Downloading BoxKit-2023.6.7.tar.gz (852 kB)
     ---------------------------------------- 852.5/852.5 kB 1.9 MB/s eta 0:00:00
  Preparing metadata (setup.py) ... error
  error: subprocess-exited-with-error

  Γ— python setup.py egg_info did not run successfully.
  β”‚ exit code: 1
  ╰─> [10 lines of output]
      Traceback (most recent call last):
        File "<string>", line 2, in <module>
        File "<pip-setuptools-caller>", line 34, in <module>
        File "C:\Users\MENDR\AppData\Local\Temp\1\pip-install-jv7ymwcp\boxkit_f17c2e8a1aef4428bab1ef1a7d2994e5\setup.py", line 14, in <module>
          import bin.cmd as bin_cmd  # pylint: disable=wrong-import-position
        File "C:\Users\MENDR\AppData\Local\Temp\1\pip-install-jv7ymwcp\boxkit_f17c2e8a1aef4428bab1ef1a7d2994e5\bin\cmd.py", line 13, in <module>
          from cbox import cbox_build  # pylint: disable=wrong-import-position
        File "C:\Users\MENDR\AppData\Local\Temp\1\pip-install-jv7ymwcp\boxkit_f17c2e8a1aef4428bab1ef1a7d2994e5\bin\cbox.py", line 21, in <module>
          if os.path.exists(os.getenv("PWD") + "/boxkit/depends/boost")
      TypeError: unsupported operand type(s) for +: 'NoneType' and 'str'
      [end of output]

  note: This error originates from a subprocess, and is likely not a problem with pip.
error: metadata-generation-failed

Γ— Encountered error while generating package metadata.
╰─> See above for output.

note: This is an issue with the package mentioned above, not pip.
hint: See above for details.

(py310) C:\Users\MENDR>pip install BoxKit
Collecting BoxKit
  Using cached BoxKit-2023.6.7.tar.gz (852 kB)
  Preparing metadata (setup.py) ... error
  error: subprocess-exited-with-error

  Γ— python setup.py egg_info did not run successfully.
  β”‚ exit code: 1
  ╰─> [10 lines of output]
      Traceback (most recent call last):
        File "<string>", line 2, in <module>
        File "<pip-setuptools-caller>", line 34, in <module>
        File "C:\Users\MENDR\AppData\Local\Temp\1\pip-install-r2sw90yv\boxkit_decd8282603049f5bb2d36ea2947c869\setup.py", line 14, in <module>
          import bin.cmd as bin_cmd  # pylint: disable=wrong-import-position
        File "C:\Users\MENDR\AppData\Local\Temp\1\pip-install-r2sw90yv\boxkit_decd8282603049f5bb2d36ea2947c869\bin\cmd.py", line 13, in <module>
          from cbox import cbox_build  # pylint: disable=wrong-import-position
        File "C:\Users\MENDR\AppData\Local\Temp\1\pip-install-r2sw90yv\boxkit_decd8282603049f5bb2d36ea2947c869\bin\cbox.py", line 21, in <module>
          if os.path.exists(os.getenv("PWD") + "/boxkit/depends/boost")
      TypeError: unsupported operand type(s) for +: 'NoneType' and 'str'
      [end of output]

  note: This error originates from a subprocess, and is likely not a problem with pip.
error: metadata-generation-failed

Γ— Encountered error while generating package metadata.
╰─> See above for output.

note: This is an issue with the package mentioned above, not pip.
hint: See above for details.

I think this maybe originating from the command os.getenv("PWD") being executed on a Windows Operating System. $PWD only exists on Linux/Unix based Operating Systems. I will write a fix for this.

Thank you for reporting this bug.

akashdhruv commented 11 months ago

Can you try pip install boxkit==2023.9

rvg296 commented 11 months ago

Yes, I have tried. Here's what I get and I believe its related to pathname

`Collecting boxkit==2023.9
  Downloading BoxKit-2023.9.0.tar.gz (1.4 MB)
     ---------------------------------------- 1.4/1.4 MB 3.2 MB/s eta 0:00:00
  Preparing metadata (setup.py) ... error
  error: subprocess-exited-with-error

  Γ— python setup.py egg_info did not run successfully.
  β”‚ exit code: 1
  ╰─> [10 lines of output]
      Traceback (most recent call last):
        File "<string>", line 2, in <module>
        File "<pip-setuptools-caller>", line 34, in <module>
        File "C:\Users\MENDR\AppData\Local\Temp\1\pip-install-zydhqfqy\boxkit_9dbb73bbaeb0445ca87594bacf0d2c0f\setup.py", line 49, in <module>
          packages=find_packages(where="./"),
        File "C:\Users\MENDR\AppData\Local\ESRI\conda\envs\py310\lib\site-packages\setuptools\discovery.py", line 127, in find
          convert_path(str(where)),
        File "C:\Users\MENDR\AppData\Local\ESRI\conda\envs\py310\lib\site-packages\setuptools\_distutils\util.py", line 141, in convert_path
          raise ValueError("path '%s' cannot end with '/'" % pathname)
      ValueError: path './' cannot end with '/'
      [end of output]

  note: This error originates from a subprocess, and is likely not a problem with pip.
error: metadata-generation-failed

Γ— Encountered error while generating package metadata.
╰─> See above for output.

note: This is an issue with the package mentioned above, not pip.
hint: See above for details.`
akashdhruv commented 11 months ago

It looks like I will have to fix the source code to be able to work with either windows or linux machines. I originally intended BoxKit to be used on HPC clusters which are largely linux based.

I have setup a minimal test to replicate your windows test problem: https://github.com/Box-Tools/BoxKit/actions/runs/6343640315

akashdhruv commented 11 months ago

@rvg296 It has been fixed. See: https://github.com/Box-Tools/BoxKit/actions/runs/6344172628/job/17233705158?pr=144

Can you try, pip install boxkit==2023.9.1 --user

and also, git+ssh://git@github.com/akashdhruv/BoxKit.git@main --user

rvg296 commented 11 months ago

@akashdhruv That worked, thanks for fixing that.

akashdhruv commented 11 months ago

Perfect. Thank you for reporting the bug.

rvg296 commented 11 months ago

Please update this link (https://akashdhruv.github.io/BoxKit/) in BoxKit docs wherever related.

akashdhruv commented 11 months ago

Please update this link (https://akashdhruv.github.io/BoxKit/) in BoxKit docs wherever related.

Its already up to date on the repository page: https://github.com/Box-Tools/BoxKit

rvg296 commented 11 months ago

I was referring to these, under contribution and usage.

akashdhruv commented 11 months ago

I was referring to these, under contribution and usage.

Done.

rvg296 commented 11 months ago

@akashdhruv , In the paper, you have mentioned the performance is improved ~5x for measuring the bubble shape and size for Flash-X boiling simulations. Could you please locate that in the box-kit performance repo. Is that in the Jupyter notebook?