openjournals / joss-reviews

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

[REVIEW]: VTUFileHandler: A VTU library in the Julia language that implements an algebra for basic mathematical operations on VTU data #4300

Closed editorialbot closed 2 years ago

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@baxmittens<!--end-author-handle-- (maximilian bittens) Repository: https://github.com/baxmittens/VTUFileHandler Branch with paper.md (empty if default branch): Version: v0.2.0 Editor: !--editor-->@jbytecode<!--end-editor-- Reviewers: @dmbates, @mkitti Archive: 10.5281/zenodo.6576155

Status

status

Status badge code:

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

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

@dmbates & @mkitti, 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 @jbytecode 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 @mkitti

📝 Checklist for @dmbates

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.05 s (370.7 files/s, 30699.4 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Julia                           11            126             47            850
XML                              2              0              0            166
Markdown                         2             30              0            133
TeX                              1              3              0             36
TOML                             1              2              0             15
-------------------------------------------------------------------------------
SUM:                            17            161             47           1200
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 2 years ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.23919/EuCAP.2017.7928679 is OK
- 10.1109/38.865875 is OK
- 10.21105/joss.03673 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 2 years ago

Wordcount for paper.md is 959

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:

jbytecode commented 2 years ago

Dear @dmbates and @mkitti

This is the reviewing thread. First, please type

@editorialbot generate my checklist

to generate checklist. In this checklist there will be 20 check items for each reviewer. Whenever you complete the corresponding item, you can check them on. The reviewing process is interactive so you can always interact with the author(s) and the editor.

You can discuss issues here as well as in target the repository by opening new issues. If you open a new issue in the target repository, please mention them here so we can keep tracking what is going on outside the main thread.

Thank you in advance.

jbytecode commented 2 years ago

@baxmittens - at a first glance, it seems a citation for Julia is missing. could you please consider adding it?

@dmbates, @mkitti - could you please create your checklist by typing the editorialbot command given above?

Thank you all in advance.

mkitti commented 2 years ago

Review checklist for @mkitti

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

dmbates commented 2 years ago

Review checklist for @dmbates

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

mkitti commented 2 years ago

The authors introduce VTUFileHandler.jl, a Julia language package for handling VTK unstructured grid data. The package also provides a number of mathematical operators reminiscent of an algebraic field over addition and multiplication.

  1. Currently VTUFileHandler.jl is not a registered Julia package. It depends on another unregistered package located at https://github.com/baxmittens/XMLParser, which is a bespoke "leightweight" [spelling error] package.

Does the author intend to register one or both packages?

Given the the XMLParser module is bespoke, is it really meant to be an independent package? Could it exist as a module within VTUFileHandler.jl?

  1. Since part of the package deals with file based input and output, it would be useful if this functionality could be registered file handler via FileIO.jl

  2. The registered package WriteVTK.jl appears to implement file loading and saving for .vtu files. A comparison to this package should be done. I would also appreciate if the author could explore interoperability with this package.

  3. While the author asserts that this library "implements an algebra" I would like the author to defend this statement by addressing a few algebraic concepts. i) Are the operations associative? ii) Are the operations commutative? iii) Are there additive and multiplicative identities? iv) Are there additive and multiplicative inverses? v) Can multiplication be distributed over addition? vi) Over the set of VTUFiles do addition and multiplication form a mathematical field? v) Over the set of VTUFiles and Numbers do addition and multiplication form a multiplicative field?

  4. The method nbytes is implemented as a utility within the package: https://github.com/baxmittens/VTUFileHandler.jl/blob/633c67c0d1f50c3d75903448b7800c8fa944d407/src/VTUFileHandler/vtuheader_utils.jl#L13-L17 . This appears to replicate the functionality of sizeof

julia> nbytes(UInt8)
1

julia> sizeof(UInt32)
4

julia> sizeof(UInt64)
8

julia> sizeof(Float64)
8

julia> sizeof(Int32)
4

julia> sizeof(Int64)
8

julia> sizeof(UInt8)
1

Could the definition const nbytes = sizeof replace the current hardcoded version?

To be continued...

baxmittens commented 2 years ago

@baxmittens - at a first glance, it seems a citation for Julia is missing. could you please consider adding it?

Gladly did so.

jbytecode 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.23919/EuCAP.2017.7928679 is OK
- 10.1109/38.865875 is OK
- 10.21105/joss.03673 is OK

MISSING DOIs

- None

INVALID DOIs

- None
jbytecode 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:

baxmittens commented 2 years ago

@mkitti - thank you for your comments. I would like to answer to them

  1. Currently VTUFileHandler.jl is not a registered Julia package. It depends on another unregistered package located at https://github.com/baxmittens/XMLParser, which is a bespoke "leightweight" [spelling error] package.

Fixed the typo.

Does the author intend to register one or both packages?

Not at this point but maybe later. I elaborate on this below

Given the the XMLParser module is bespoke, is it really meant to be an independent package? Could it exist as a module within VTUFileHandler.jl?

I use it in other projects of mine too. For example for some simple html and OpenGeoSys input file manipulations. Thinking in term of functions, XMLParser.jl should be a stand-alone package since it has a distinctive function. I'm no big fan of mixing too many diffierent functionalities in one project since it complicates the expandability and the maintainbility. What I would consider is replacing my XMLParser for a better one. The problem is, to my knowledge, there is no XMLParser available which is written purely in Julia. All projects I know (LightXML.jl , EzXML.jl) use libxml2 as a basis, which I wanted to avoid.

And finally, even VTUFileHandler.jl is only part of a bigger project. As I wrote earlier (in Prereview), I implemented an adaptive sparse-grid colloction method (see sparse grid theory paper) which uses the VTUFileHandler and a broader stochastic / hpc framework which uses the adaptive sparse-grid. I could throw this all in one package but I thought it would be much better to distribute this big project in smaller packages according to their functionalities. I plan on publishing two consecutive joss-papers describing the sparse-grid and the stochastic-hpc-framework

  1. Since part of the package deals with file based input and output, it would be useful if this functionality could be registered file handler via FileIO.jl
  2. The registered package WriteVTK.jl appears to implement file loading and saving for .vtu files. A comparison to this package should be done. I would also appreciate if the author could explore interoperability with this package.

Before starting this project, I looked into both WriteVTK.jl and ReadVTK.jl and at that point both package were not really a good fit for what I was planning on doing. But as things progress I would definetly consider rebasing my stuff onto their projects. That is a reason why I don't think VTUFileHanlder.jl should be a registered julia package at the moment.

  1. While the author asserts that this library "implements an algebra" I would like the author to defend this statement by addressing a few algebraic concepts. i) Are the operations associative? ii) Are the operations commutative? iii) Are there additive and multiplicative identities? iv) Are there additive and multiplicative inverses? v) Can multiplication be distributed over addition? vi) Over the set of VTUFiles do addition and multiplication form a mathematical field? v) Over the set of VTUFiles and Numbers do addition and multiplication form a multiplicative field?

As mentioned in the paper, if operators should be applied the objects have to share the same topology. If they do, all operators act point-wise on each corresponding pair of e.g. nodal coefficients. The operators itself keep their properties as they perform their action on a collection of scalars. Each nodal coefficient $\hat{u}\in\mathbb{R}$ is an independet entity.

  1. The method nbytes is implemented as a utility within the package: https://github.com/baxmittens/VTUFileHandler.jl/blob/633c67c0d1f50c3d75903448b7800c8fa944d407/src/VTUFileHandler/vtuheader_utils.jl#L13-L17 . This appears to replicate the functionality of sizeof

Thank you. Changed all nbytes to sizeof. That makes a lot more sense...

Sorry for the long answer.

jbytecode commented 2 years ago

Looks like there are lots of "my needs" instead of "community's needs", the software can be refactored in lights of the reviews.

The dependency XMLParser as declared in the Project.toml is not a standard Julia package - I am sharing the same thoughts with the reviewer.

Altought it is not strictly needed, we always encourage authors to submit their packages so it fits well with the eco-system of the language and the environments.

At the end of the story, the software should solve a generalized problem with an easy and painless installation process.

baxmittens commented 2 years ago

Looks like there are lots of "my needs" instead of "community's needs", the software can be refactored in lights of the reviews.

The dependency XMLParser as declared in the Project.toml is not a standard Julia package - I am sharing the same thoughts with the reviewer.

Altought it is not strictly needed, we always encourage authors to submit their packages so it fits well with the eco-system of the language and the environments.

At the end of the story, the software should solve a generalized problem with an easy and painless installation process.

Agree, this was a project that originally was created only in my needs and I was asked to open-source it because other people wanted to use it. But I do think it solves a generalized problem quite handy. And I also think the installation process is painless since no package has dependencies which is quite nice if you think about computations on heterogenous cluster architectures.

mkitti commented 2 years ago

Before starting this project, I looked into both WriteVTK.jl and ReadVTK.jl and at that point both package were not really a good fit for what I was planning on doing. But as things progress I would definetly consider rebasing my stuff onto their projects. That is a reason why I don't think VTUFileHandler.jl should be a registered julia package at the moment.

Regarding package registration, I do not see how dependence on the other packages or lack there of would prevent package registration. This can always be modified in subsequent vesions of the package. VTUFileHandler.jl is either a reusable library or a terminal application. 1) A reusable library should be in a registry of some sort to facilitate installation. 2) Alternatively, a terminal application should have a Manifest.toml for reproducibility as well as a DOI via a service like Zenodo.

Per the review criteria on https://joss.readthedocs.io/en/latest/review_criteria.html#installation-instructions:

Where possible, software installation should be managed with a package manager. However, this criterion depends upon the maturity and availability of software packaging and distribution in the programming language being used. For example, Python packages should be pip installable, and have adopted packaging conventions.

Julia has a mature software packaging system and for publication VTUFileHandler.jl should take full advantage of it. I will let the author decide between the two options above, but one of them must be chosen as a requirement of publication. The choice will also affect how to integrate the XMLParser module.

See https://github.com/JuliaRegistries/General#should-i-register-my-package

mkitti commented 2 years ago

7. While the author asserts that this library "implements an algebra" I would like the author to defend this statement by addressing a few algebraic concepts. i) Are the operations associative? ii) Are the operations commutative? iii) Are there additive and multiplicative identities? iv) Are there additive and multiplicative inverses? v) Can multiplication be distributed over addition? vi) Over the set of VTUFiles do addition and multiplication form a mathematical field? v) Over the set of VTUFiles and Numbers do addition and multiplication form a multiplicative field?

As mentioned in the paper, if operators should be applied the objects have to share the same topology. If they do, all operators act point-wise on each corresponding pair of e.g. nodal coefficients. The operators itself keep their properties as they perform their action on a collection of scalars. Each nodal coefficient $\hat{u}\in\mathbb{R}$ is an independet entity.

Thank you for this comment. Please clarify the above points in the paper, clearly delineating how this library implements an algebra such as a field. If you feel there is an alternative algebra, please state what it is. Specifically there are a couple missing parts I have identified so far:

  1. Additive inverse with a unary - operator.
  2. Multiplicative identity (e.g. Base.one).
mkitti commented 2 years ago

The problem is, to my knowledge, there is no XMLParser available which is written purely in Julia. All projects I know (LightXML.jl , EzXML.jl) use libxml2 as a basis, which I wanted to avoid.

It would be good to document this in the README for XMLParser.

baxmittens commented 2 years ago

Regarding package registration, I do not see how dependence on the other packages or lack there of would prevent package registration. This can always be modified in subsequent vesions of the package. VTUFileHandler.jl is either a reusable library or a terminal application.

  1. A reusable library should be in a registry of some sort to facilitate installation.
  2. Alternatively, a terminal application should have a Manifest.toml for reproducibility as well as a DOI via a service like Zenodo.

Per the review criteria on https://joss.readthedocs.io/en/latest/review_criteria.html#installation-instructions:

Where possible, software installation should be managed with a package manager. However, this criterion depends upon the maturity and availability of software packaging and distribution in the programming language being used. For example, Python packages should be pip installable, and have adopted packaging conventions.

Julia has a mature software packaging system and for publication VTUFileHandler.jl should take full advantage of it. I will let the author decide between the two options above, but one of them must be chosen as a requirement of publication. The choice will also affect how to integrate the XMLParser module.

See https://github.com/JuliaRegistries/General#should-i-register-my-package

Sorry but I disagree, here. My package already meets the review criteria (Good: The software is simple to install, and follows established distribution and dependency management approaches for the language being used).

Via

import Pkg
Pkg.add(url="https://github.com/baxmittens/XMLParser.jl.git")
Pkg.add(url="https://github.com/baxmittens/VTUFileHandler.jl.git")

the package is registered in the julia package manager. it will be automatically updated if Pkg.update() is called and a new version is available. All dependencies are listed in the Project.toml.

A Manifest.toml is not needed here. You can think of the Manifest as a fixed snapshot of the Project.toml which is needed if your package requires specific versions/releases of its dependecies for a given application. In other words for specific applications it can be necessary that the user has the very same environment and thus a Manifest.toml has to be included.

This is not necessary here since my package should work with all versions of julia 1.x and only uses Julia.Base besides the XMLParser. I always thought that in that case you should avoid to add a Manifest.toml since this would result in overly restricting the versions of the package dependencies and leads to large .julia folders.

In some cases a test/Manifest.toml seems appropriate but in my case I don't see why. Only in case incompatibilites with other packages are occuring, I should add a Manifest.toml. Right?

I always thought this would be good julia practice. See for example timholy's projects.

Edit: While reading further into the topic, I don't know if the above is right. It seems like the Manifest.toml would be added automatically, if the I would register the Project in the Julia package manager. But still, adding a Manifest.toml manually to the repo doesn't make sense, right?

baxmittens commented 2 years ago

The problem is, to my knowledge, there is no XMLParser available which is written purely in Julia. All projects I know (LightXML.jl , EzXML.jl) use libxml2 as a basis, which I wanted to avoid.

It would be good to document this in the README for XMLParser.

Thank you for the suggestion. Added it in the readme.

baxmittens commented 2 years ago
  1. While the author asserts that this library "implements an algebra" I would like the author to defend this statement by addressing a few algebraic concepts. i) Are the operations associative? ii) Are the operations commutative? iii) Are there additive and multiplicative identities? iv) Are there additive and multiplicative inverses? v) Can multiplication be distributed over addition? vi) Over the set of VTUFiles do addition and multiplication form a mathematical field? v) Over the set of VTUFiles and Numbers do addition and multiplication form a multiplicative field?

As mentioned in the paper, if operators should be applied the objects have to share the same topology. If they do, all operators act point-wise on each corresponding pair of e.g. nodal coefficients. The operators itself keep their properties as they perform their action on a collection of scalars. Each nodal coefficient $\hat{u}\in\mathbb{R}$ is an independet entity.

Thank you for this comment. Please clarify the above points in the paper, clearly delineating how this library implements an algebra such as a field. If you feel there is an alternative algebra, please state what it is. Specifically there are a couple missing parts I have identified so far:

Let V=(VTUFile,+,*) be a field and A \subseteq R^n a vector space over V. Then V is an algebra if for all x,y,z \in A and a,b \in V the following holds:

  1. (x+y) z = x z + y * z
  2. z (x+y) = z x + z * y
  3. (ax) (bx) = (ab)(x y)

which surely applies but I don't really think this should be included in the paper or should it?

  1. Additive inverse with a unary - operator.
  2. Multiplicative identity (e.g. Base.one).

Added those, thank you.

mkitti commented 2 years ago

I don't really think this should be included in the paper or should it?

The language of the title is very specific. So yes, I think the contents of the a article should support the statement in the title.

baxmittens commented 2 years ago

The language of the title is very specific. So yes, I think the contents of the a article should support the statement in the title.

I would also consider changing the title to something like

A VTU library in the Julia language that implements algebraic operators on VTU data

if the language is too specific otherwise (I'm no native speaker).

We can also stick to the title and I define an algebra in the paper if this is the better solution

mkitti commented 2 years ago

You can think of the Manifest as a fixed snapshot of the Project.toml which is needed if your package requires specific versions/releases of its dependecies for a given application. In other words for specific applications it can be necessary that the user has the very same environment and thus a Manifest.toml has to be included.

Exactly. You have submitted this software for publication, and anyone reading the article should be able to reproduce the environment down to the specific versions of all the dependencies. This will actually simplify installation because the source of the XMLParser package will be encoded in the Manifest.toml:

[[deps.XMLParser]]
git-tree-sha1 = "5ffa66df38a330a7be8584dd8058e6f07fac928c"
repo-rev = "main"
repo-url = "https://github.com/baxmittens/XMLParser.jl.git"
uuid = "b09a1ce3-015a-45b4-89ae-f27efbd2c76d"
version = "0.0.4"

You could consider putting this on a specific branch related to this article. If this article is published, you could then add a tag at the time for publication, and then use Zenodo to acquire a permanent snapshot of the software at that tag which will have an associated DOI.

An application of the Manifest.toml is reproducibility for scientific publication. A second application is to capture unregistered dependencies. Thus, I believe a Manifest.toml is warranted here.

I would accept registration of the packages in lieu of this, but there is actually nothing stopping you from doing both. Your repository can have multiple branches. For the branch that you register, I would probably not include a Manifest.toml so that it can be easily combined with future versions of Julia and other packages.

mkitti commented 2 years ago

Documentation

The documentation currently consists of a single README.md. It currently has the following sections:

The package overall needs more comprehensive documentation, and I recommend the use of https://github.com/JuliaDocs/Documenter.jl for this. The author may want to use the template located at https://github.com/invenia/PkgTemplates.jl/tree/master/templates/docs to get started.

The following sections need improvement:

Statement of Need

This is partially addressed in the introduction. Perhaps it should borrow more from the corresponding section in the paper. A discussion of what this package does beyond ReadVTK.jl and WriteVTK.jl is needed as discussed above and why that functionality is needed.

Installation instructions

The installation instructions work but as detailed above they could streamlined to meet the standards for software published in JOSS given the existing packaging framework in Julia.

Dependencies are not clearly listed. The Project.toml does not include version compat information for external packages. What is the minimum version of Julia required? Is it compatible with Julia 1.0.x? Julia 1.6.x LTS?

Example usage

The example includes a misspelling for "uncrompress".

The example is incomplete. It does not include using or import statements to execute the example. The author should consider exporting commonly used symbols to simplify usage.

A link to vox8.vtu in the test folder would be useful.

It currently covers some very simple arithmetic operations, mostly with scalars. The example should be expanded to include more operations between instances of VTUFile.

Ideally, an additional real world example beyond a simple cube would be included.

Instructions for how to generate the actual plot depicted should be included.

Functionality documentation

Comprehensive API documentation is absent from the README. The code itself does contain doc strings, but due to to an extra space between the docstrings and the identifier, the doc strings are not correctly associated.

For example, the following line should be removed. Alternatively, VTUHeader could be placed there to associate the doc string. https://github.com/baxmittens/VTUFileHandler.jl/blob/3aa96c4b15f155de4b73d826bcf0fa4e30f79e2b/src/VTUFileHandler.jl#L17

help?> VTUFileHandler.VTUHeader
  No documentation found.

  Summary
  ≡≡≡≡≡≡≡≡≡

  struct VTUFileHandler.VTUHeader{T<:Union{UInt32, UInt64}}

  Fields
  ≡≡≡≡≡≡≡≡

  num_blocks            :: T<:Union{UInt32, UInt64}
  blocksize             :: T<:Union{UInt32, UInt64}
  last_blocksize        :: T<:Union{UInt32, UInt64}
  compressed_blocksizes :: Array{T<:Union{UInt32, UInt64}, 1}

Documenter.jl would be easiest way to export the docstrings into readable documentation.

Automated testing

There are automated tests but they are not described in the documentation. Additional code coverage is needed. The tests are not part of a continuous integration system and thus are not fully automated.

This template for Github Actions may be useful to setup CI: https://github.com/invenia/PkgTemplates.jl/blob/master/templates/github/workflows/CI.yml

Community guidelines

There are no community guidelines in the documentation.

There should be clear guidelines for third-parties wishing to:

Contribute to the software Report issues or problems with the software Seek support

mkitti commented 2 years ago

Paper

Summary

There is not a clear description what a VTU file is other than naming it as "VTK unstructured grid". An introduction to this would be useful to make this paper more accessible to a non-specialist audience.

The author also leads with the motivation about "discrete numerical simulations". The software is likely has more general applicability than "discrete numerical simulations". Perhaps this description could be made to appeal to a wider audience.

Statement of need

A statement of need section does exist in the paper 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 and who the target audience is?

State of the field

This section is absent. There appears to be other VTK related I/O packages available and those should be discussed.

Quality of the Writing

The language itself is good, however the structure of the writing seems to dive into the details of a specific application before addressing what the package actual does. For a non-specialist audience would be good to elaborate on some of the basics first:

1) What is VTK? 2) What is "VTK unstructured grid data"? 3) How do I plot "VTK unstructured grid data"? 4) What is the prior work in the field for similar packages in Julia and in other languages? Similar packages in Python are only mentioned in the very last sentence of the conclusion. 5) What is Julia? 6) What does this package allow the user to do?

What is currently in the Introduction perhaps belongs in a Motivation section in a subsequent section after a basic introduction.

References

VTK itself is not properly cited as requested.

Schroeder, Will; Martin, Ken; Lorensen, Bill (2006), The Visualization Toolkit (4th ed.), Kitware, ISBN 978-1-930934-19-1

mkitti commented 2 years ago

VTUData and several other struct types are declared to be mutable but it is not clear to me that they need to be. All of VTUData's fields are Vectors, which themselves are mutable. Why is VTUData mutable?

baxmittens commented 2 years ago

@mkitti - thank you for the detailed explanations. I will need some time to process them and alter my projects accordingly.

jbytecode commented 2 years ago

a small note for tests:

The Test package includes useful macros for testing functions provided by the package and a standard testing scheme is

@testset "Test Description" begin
      // Some calculations 
     result = somefunction(...)
     @test result == expected
     @test isapprox(...)
end 

in Julia. The authors may consider refactoring their test files using the standard Julia scheme.

Here is an example: https://github.com/jmejia8/Metaheuristics.jl/blob/master/test/common-methods.jl

baxmittens 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:

baxmittens commented 2 years ago

@mkitti - tried to adapt the paper according to your suggestions.

I am about adding the packages to the julia package manager and adding documentation. will still take me a while. Im struggling with github actions and other stuff here : )

mkitti commented 2 years ago

I suggest asking https://discourse.julialang.org for the items you are struggling with.

The chat resources here can provide some interactive feedback as well: https://julialang.org/community/

Package registration can be done via a web form if preferred: https://github.com/JuliaRegistries/Registrator.jl

To get started I usually would use https://github.com/invenia/PkgTemplates.jl in interactive mode.

jbytecode commented 2 years ago

@baxmittens - could you please update your status and inform us? any problems?

baxmittens commented 2 years ago

@jbytecode - Following mkitti's and your recommendations, I have registered both GitHub projects as Julia packages and added documentations. I added a section to the paper and did some alterations to the code. I have to write some unit test now, define a test enviroment and do some minor things here and there and then I think I will be finished. This will take me until next week, I guess.

mkitti commented 2 years ago

I have been following the progress XMLParser.jl: https://github.com/JuliaRegistries/General/pull/58868 VTUFileHandler.jl: https://github.com/JuliaRegistries/General/pull/59660 Documenter.jl based documentation: https://baxmittens.github.io/VTUFileHandler.jl/dev/

I have not seen contribution and support guidelines yet though.

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

baxmittens commented 2 years ago

@mkitti - I added a CI.yml and a branch joss_paper with a Manifest.toml. Is this sufficient or do I need to include Zenodo as well? And if so, how is it done? Do I upload a zip-file of this branch? I'm not quite sure.

jbytecode commented 2 years ago

You don't need a Zenodo archive in this stage.

mkitti commented 2 years ago

Package registration satisfies my prior concerns. The Manifest.toml is appreciated. Zenodo would be most applicable at publication time.

baxmittens commented 2 years ago

@mkitti @jbytecode - As far as I'm aware, I think I have fulfilled all of your requests. Maybe you could have a look and check if this is actually true? Would be nice. But no hurries. I have plenty other work, right now. Thank you in advance.

mkitti commented 2 years ago

@baxmittens the last render of the paper is 25 days old. Perhaps we just need to rerender the article?

mkitti 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:

jbytecode commented 2 years ago

@dmbates - could you please update your review status? Thank you in advance!

mkitti commented 2 years ago

Overall, the paper, software, and documentation have greatly improved during the course of this review. The submitted paper describes a valuable tool to process VTU files from the VTK framework. It provides arithmetic operations on the types representing the files that complete an algebraic field. Additionally, a supporting package has an accessory package, XMLParser.jl that provides a lightweight XML framework. The software is easy to install. The paper introduces the background to a general audience and describes the main functionality of the paper. I thus recommend the paper for publication.

I have listed a few minor points below that I believe can be easily addressed. I may a few additional minor recommendations after I do a close reading of the paper later this week.

Minor Points

1. Code or command to generate Figure 1

I would appreciate if the author could provide code to generate Figure 1 or at least view the VTU file in VTK. This could also be a command line invocation on the VTU file.

2. Community Guidelines

The current community guidelines listed are as follows:

Contributions to or questions about this project are welcome. Feel free to create a issue or a pull request.

I would clarify that you would like them to create an issue or pull request on Github.

It's possible that the README may be read on other websites other than Github. For example: https://juliahub.com/ui/Packages/VTUFileHandler/udkbV/0.1.3

3. Link back to the repository from the documentation

Similar to the above, also please provide a link from the documentation to the Github repository since the documentation may be read on sites other than Github or even offline.

https://docs.juliahub.com/VTUFileHandler/udkbV/0.1.3/

4. Document implemented base methods

I would appreciate a brief listing of what Base methods are implemented for VTUFile in the documentation would be useful.

An incomplete list is as follows:

baxmittens commented 2 years ago

@mkitti -

1. Code or command to generate Figure 1

I do not use any code to render the images. maybe this is possible with some additional vtk-software. I just use paraview and insert the files manually. i added that to the paper. Is that ok?

I think I have implemented the rest of the aforementioned demands.

baxmittens commented 2 years ago

@editorialbot generate pdf