openjournals / joss-reviews

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

[REVIEW]: Mechkit: A continuum mechanics toolkit in Python #4389

Closed editorialbot closed 1 year ago

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@JulianKarlBauer<!--end-author-handle-- (Julian Karl Bauer) Repository: https://github.com/JulianKarlBauer/mechkit Branch with paper.md (empty if default branch): paper Version: v0.4.0 Editor: !--editor-->@Kevin-Mattheus-Moerman<!--end-editor-- Reviewers: @nicoguaro, @likask, @lizarett Archive: 10.5281/zenodo.7185691

Status

status

Status badge code:

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

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

@nicoguaro & @likask & @lizarett, 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 @Kevin-Mattheus-Moerman 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 @nicoguaro

📝 Checklist for @likask

📝 Checklist for @lizarett

editorialbot commented 2 years 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 2 years ago
Software report:

github.com/AlDanial/cloc v 1.88  T=0.09 s (770.0 files/s, 148370.7 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
SVG                              3              3              3           3679
Python                          24           1021           1385           2969
YAML                             6             22             10            460
TeX                              3             42             19            457
Markdown                         9             67              0            235
make                             2             35             13            188
Jupyter Notebook                 6              0           1683            179
reStructuredText                13             74            121             52
-------------------------------------------------------------------------------
SUM:                            66           1264           3234           8219
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 2 years ago

Wordcount for paper.md is 821

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

OK DOIs

- 10.1177/10812865211057602 is OK
- 10.1016/0020-7225(84)90090-9 is OK
- 10.1007/978-3-319-19566-7 is OK
- 10.1007/978-1-4757-1275-9 is OK
- 10.1093/qjmam/43.1.15 is OK
- 10.1016/0022-5096(92)90029-2 is OK
- 10.1088/978-0-7503-1454-1 is OK
- 10.5281/zenodo.4679756 is OK
- 10.5281/zenodo.1173115 is OK
- 10.5281/zenodo.5938012 is OK
- 10.5281/zenodo.5564818 is OK

MISSING DOIs

- 10.1016/0020-7225(70)90024-8 may be a valid DOI for title: A note on the decomposition of tensors into traceless symmetric tensors
- 10.1122/1.549945 may be a valid DOI for title: The use of tensors to describe and predict fiber orientation in short fiber composites

INVALID DOIs

- None
editorialbot commented 2 years ago

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

Kevin-Mattheus-Moerman commented 2 years ago

@JulianKarlBauer can you check those potentially missing DOI's :point_up: ?

JulianKarlBauer commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

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

JulianKarlBauer commented 2 years ago

@JulianKarlBauer can you check those potentially missing DOI's point_up ?

Thank you for pointing this out! I added the missing DOIs and regenerated the pdf.

Kevin-Mattheus-Moerman commented 2 years ago

@editorialbot check references

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

OK DOIs

- 10.1177/10812865211057602 is OK
- 10.1016/0020-7225(84)90090-9 is OK
- 10.1007/978-3-319-19566-7 is OK
- 10.1007/978-1-4757-1275-9 is OK
- 10.1093/qjmam/43.1.15 is OK
- 10.1016/0022-5096(92)90029-2 is OK
- 10.1088/978-0-7503-1454-1 is OK
- 10.1122/1.549945 is OK
- 10.5281/zenodo.4679756 is OK
- 10.5281/zenodo.1173115 is OK
- 10.5281/zenodo.5938012 is OK
- 10.5281/zenodo.5564818 is OK

MISSING DOIs

- None

INVALID DOIs

- https://doi.org/10.1016/0020-7225(70)90024-8 is INVALID because of 'https://doi.org/' prefix
Kevin-Mattheus-Moerman commented 2 years ago

@JulianKarlBauer nearly there, note the invalid one :point_up:, fyi you can also run @editorialbot check references.

JulianKarlBauer commented 2 years ago

@editorialbot check references

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

OK DOIs

- 10.1177/10812865211057602 is OK
- 10.1016/0020-7225(84)90090-9 is OK
- 10.1007/978-3-319-19566-7 is OK
- 10.1007/978-1-4757-1275-9 is OK
- 10.1093/qjmam/43.1.15 is OK
- 10.1016/0022-5096(92)90029-2 is OK
- 10.1088/978-0-7503-1454-1 is OK
- 10.1016/0020-7225(70)90024-8 is OK
- 10.1122/1.549945 is OK
- 10.5281/zenodo.4679756 is OK
- 10.5281/zenodo.1173115 is OK
- 10.5281/zenodo.5938012 is OK
- 10.5281/zenodo.5564818 is OK

MISSING DOIs

- None

INVALID DOIs

- None
Kevin-Mattheus-Moerman commented 2 years ago

@nicoguaro, @likask, @lizarett this is where the review takes place. You can each call @editorialbot generate my checklist here to create your check list. Let me know if you have any question.

nicoguaro commented 2 years ago

Review checklist for @nicoguaro

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

nicoguaro commented 2 years ago

I have done a first pass on the package. I have some comments:

I will resume the review after these comments have been addressed.

JulianKarlBauer commented 2 years ago

Hi @nicoguaro thank you very much for starting your review! Please let me address the points of concern.


* There isn't a section or file specifying how to contribute to the project.

Thank you, indeed a file CONTRIBUTING.md only had been added to the paper-branch, which had been used to create the paper draft and prepare the review. I merged the paper-branch into master and will keep both identical side by side. As I specified the paper-branch during submission to JOSS, I'll keep it alive till the end of the review.


* I don't see a clear statement of need on the README or docs.

This is true, I copied the "Statement of need" from the paper to the landing page of the docs. Personally I think that the README.md should not contain a statement of need, this is why I added it to the docs. Is this ok for you? (see b8052796ed936d32b8ace1160bea27f5cc596458)


* There isn't an API section on the docs. My guess is that it is already there in the different sections that appear on ReadTheDocs.

I added headings to the different parts of the documentation now looking like: image

Is this ok from your point of view?


Thank you again for starting the review and the valuable feedback!

Kevin-Mattheus-Moerman commented 2 years ago

@likask, @lizarett thanks for your help with this review. Can you please provide an update on progress? Thanks.

Kevin-Mattheus-Moerman commented 2 years ago

@likask, @lizarett thanks for your help with this review. Can you please provide an update on progress? Thanks.

nicoguaro commented 2 years ago

Is this ok from your point of view?

It is better. Although, I would add an API section in the left menu bar to group all the different modules.

Regarding the paper, I don't see any mention of similar packages. I do not know if there are any besides SymPy's module (not so similar) and continuum_mechanics (disclaimer, I am the author).

JulianKarlBauer commented 2 years ago

Is this ok from your point of view?

It is better. Although, I would add an API section in the left menu bar to group all the different modules.

Thanks, I'll fight with Sphinx to clean the lean toctree.

Regarding the paper, I don't see any mention of similar packages. I do not know if there are any besides SymPy's module (not so similar) and continuum_mechanics (disclaimer, I am the author).

Thank you for mentioning these packages which also refer to continuum mechanics. However, the tasks accomplished by these packages are quite varying. This is probably caused by the fact that the phrase continuum mechanics describes a high-level topic. As far as I know, no packages similar to mechkit exist. This is the reason, why I did not list others.

SymPy's module implements beam-theory and therefore should refer to "structural mechanics" instead of continuum mechanics. Continuum_mechanics is a great package for a rather specific selection of continuum mechanics topics, like mechkit. But as far as I see, no common tasks are solved.

Kevin-Mattheus-Moerman commented 2 years ago

@likask, @lizarett can you please update us on review progress? Thanks again for your help!

Kevin-Mattheus-Moerman commented 2 years ago

@likask, @lizarett can you please update us on review progress? Thanks again for your help!

:point_up:

Kevin-Mattheus-Moerman commented 2 years ago

@nicoguaro has @JulianKarlBauer address your comments to your satisfaction? :point_up:

nicoguaro commented 2 years ago

@nicoguaro has @JulianKarlBauer address your comments to your satisfaction? ☝️

@Kevin-Mattheus-Moerman, partially, I would argue that it should be clearer what is the difference with other packages. At least, since the name includes a broad topic Mechkit: A continuum mechanics toolkit in Python. I am still pending on checking the code and the tests.

JulianKarlBauer commented 2 years ago

Thank you @nicoguaro for pointing out, that the comparison with other packages should be extended.

After having looked at all 23 repositories tagged with "continuum-mechanics" on Github, it becomes clear, that our repositories are quite isolated when it comes to Python packages for continuum-mechanics. Most of the remaining 21 repositories are even more specific and only a few are written in Python, as Fortran is standard in the finit-element-method context.

If you don't mind, I'll focus on comparison to Sympy and your package and propose adding the following to the paper draft:

" The number of Python packages in the area of continuums mechanic is quite small. In addition to the symbolic treatment of structural mechanics problems with Sympy, the Python package continuum_mechanics supports visualization of second-order tensors as well as compact representation of coordinate transformations and tensor analysis operators. Compared to these packages, mechkit focuses on notation, material containers and algebraic operators, and provides these methods as a toolkit for dependent Python packages such as mechmean. "

If this addition with suitable citation is ok for you, I'll propose it to my co-authors and change the pdf.

nicoguaro commented 2 years ago

@Kevin-Mattheus-Moerman, I have reviewed the software taking into account the end-user and contributor perspectives. Following I mention some points regarding this submission:

Regarding the lack of reviews (besides mine) should we ask on the Computational Mechanics community on Twitter?

Kevin-Mattheus-Moerman commented 2 years ago

@likask, @lizarett can you please update us on review progress? Thanks

Kevin-Mattheus-Moerman commented 2 years ago

@nicoguaro thanks for your comments and suggestions. On recruiting other reviewers, I'll give the reviewers another chance to respond here before I would resort to finding alternative reviewers.

likask commented 2 years ago

Review checklist for @likask

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

likask commented 2 years ago

@JulianKarlBauer My initial impression is that it is valuable code. I might use it in my work or recommend it to my colleagues. The key is that transformations related to various notations used in continuum mechanics are tested, and implementing them from scratch in generic tensor code consumes time for validation.

More generic comments for the future:

Kevin-Mattheus-Moerman commented 2 years ago

@lizarett We can wait for your portion of the review if you commit to it now, but we may need to proceed without your review unless we hear from you within the next couple of days.

lizarett commented 2 years ago

The code appears to offer useful functionality of conversion from one notation to the other, examples help understanding the use cases. A possible suggestion would be to have in the description a sort of summary table or a list of all new function names with a list of all corresponding input variables. This may be just one more way to learn the package faster.

Kevin-Mattheus-Moerman commented 2 years ago

@JulianKarlBauer can you work on the above comments by @likask and @lizarett :point_up: ?

Kevin-Mattheus-Moerman commented 2 years ago

@lizarett can you generate your check list (call @editorialbot generate my checklist) and proceed?

lizarett commented 2 years ago

Review checklist for @lizarett

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

JulianKarlBauer commented 2 years ago

@JulianKarlBauer My initial impression is that it is valuable code. I might use it in my work or recommend it to my colleagues. The key is that transformations related to various notations used in continuum mechanics are tested, and implementing them from scratch in generic tensor code consumes time for validation. ...

Thank you @likask for the valuable feedback and sorry for the late reply, I survived Covid and earned a phd in the meantime :)

More generic comments for the future:

* Interesting development would be a visualisation of tensors using packages such as PyVista, and creating a toolset for this.

* It would be interesting to see the extension for non-classical continuum problems, such as second-order continuum, micromorphic, etc., where third-order tensors are used.

Adding routines and methods for non-classical continuum problems is indeed a valuable future topic! Are you an expert in this field?


* To "Contributing guide" add description about typical contribution scheme, fork->modify->test->pull request.

Thank you for pointing out the possible improvement in CONTRIBUTING.md. I followed your advice to specify a "Fork, Modify, Test, Pull request"-workflow explicitly. Are you happy with the changes made by this pull request? If you'd like to see more improvements, please feel free to reopen issue 48.

* I would advise mentioning in the paper the difference between this package and other tensor algebra codes. Explicitly mention some packages (e.g. https://en.wikipedia.org/wiki/Tensor_software). Studying this subject, I see that mechkit offers tensor algebra functionality and tools specifically for analysis, construction, and managing description in various notations used in solid continuum mechanics. That not necessary would be clear for someone looking for a tool like this one.

Thank you motivating a comparison with other tensor packages in the paper. I opened this issue and added a references and highlighted differences to other multi-purpose tensor packages in Python in this pull request. Does this fit your intention?

JulianKarlBauer commented 2 years ago

The code appears to offer useful functionality of conversion from one notation to the other, examples help understanding the use cases. A possible suggestion would be to have in the description a sort of summary table or a list of all new function names with a list of all corresponding input variables. This may be just one more way to learn the package faster.

Thank you @lizarett for pointing out the potential to improve the documentation, I guess I have a blind spot here. Up to now my strategy has been to give reasonable examples, as examples are straight forward. Are you more interested in a kind of list of functions like in the sub-chapters of the docs or details on argument types for some of the functions?

lizarett commented 2 years ago

@JulianKarlBauer i mean list of functions collected in one place. in my view, table formats with functions-explanation-arguments work well as a part of the documentation. please, see examples here down the page https://uk.mathworks.com/help/matlab/math/choose-an-ode-solver.html , maybe, something like this is suitable.

JulianKarlBauer commented 2 years ago

@JulianKarlBauer i mean list of functions collected in one place. in my view, table formats with functions-explanation-arguments work well as a part of the documentation. please, see examples here down the page https://uk.mathworks.com/help/matlab/math/choose-an-ode-solver.html , maybe, something like this is suitable.

@lizarett Could you have a look at the overview tables for the modules material, notation and operators I added in issue?

lizarett commented 2 years ago

@JulianKarlBauer i mean list of functions collected in one place. in my view, table formats with functions-explanation-arguments work well as a part of the documentation. please, see examples here down the page https://uk.mathworks.com/help/matlab/math/choose-an-ode-solver.html , maybe, something like this is suitable.

@lizarett Could you have a look at the overview tables for the modules material, notation and operators I added in issue?

thank you, looks good to me

JulianKarlBauer commented 2 years ago

thank you, looks good to me

@lizarett, does this imply a tick within your review checklist? :)

Kevin-Mattheus-Moerman commented 2 years ago

@nicoguaro, @likask, @lizarett can you provide an update on review progress or what is still outstanding? It looks like @JulianKarlBauer has addressed several points.

@lizarett can you check if you are now happy to tick that last box and recommend acceptance?

@likask can you check the response to your comments? :point_up: https://github.com/openjournals/joss-reviews/issues/4389#issuecomment-1212835958

@nicoguaro you have some boxes unticked, can you have a look if there are any updates, or provide points that still need to be addressed?

Thanks all for your continued help!

Kevin-Mattheus-Moerman commented 2 years ago

@JulianKarlBauer apologies for the slow response from my end. Semester has started again so I was snowed under quite a bit. I have pinged the reviewers just now so hopefully the pace will pick up now again soon.

lizarett commented 2 years ago

@nicoguaro, @likask, @lizarett can you provide an update on review progress or what is still outstanding? It looks like @JulianKarlBauer has addressed several points.

@lizarett can you check if you are now happy to tick that last box and recommend acceptance?

@likask can you check the response to your comments? ☝️ #4389 (comment)

@nicoguaro you have some boxes unticked, can you have a look if there are any updates, or provide points that still need to be addressed?

Thanks all for your continued help!

all ticked

JulianKarlBauer commented 2 years ago

@JulianKarlBauer apologies for the slow response from my end. Semester has started again so I was snowed under quite a bit. I have pinged the reviewers just now so hopefully the pace will pick up now again soon.

Thank you for your help and commitment. I appreciate the volunteer work of you, lizarett, nicoguaro and likask very much!

nicoguaro commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

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

nicoguaro commented 2 years ago

@nicoguaro you have some boxes unticked, can you have a look if there are any updates, or provide points that still need to be addressed?

All of them have been addressed except for the "state of the field". @JulianKarlBauer mentioned something before about it, but I don't see anything about it in the compiled version of the paper.