search

openjournals / joss-reviews

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

[REVIEW]: DTCC Builder: A mesh generator for automatic, efficient, and robust mesh generation for large-scale city modeling and simulation #4928

Closed editorialbot closed 1 year ago

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@alogg<!--end-author-handle-- (Anders Logg) Repository: https://github.com/dtcc-platform/dtcc-builder Branch with paper.md (empty if default branch): joss-2022 Version: v0.9 Editor: !--editor-->@crvernon<!--end-editor-- Reviewers: @ifthompson, @ipadjen Archive: 10.5281/zenodo.7988751

Status

status

Status badge code:

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

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

@pralitp & @ifthompson, 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 @crvernon 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 @ifthompson

📝 Checklist for @ipadjen

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

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

crvernon commented 2 years ago

👋 @alogg, @pralitp, @ifthompson - This is the review thread for the paper. All of our communications will happen here from now on.

Please read the "Reviewer instructions & questions" in the first comment above.

Both reviewers have checklists at the top of this thread (in that first comment) with the JOSS requirements. As you go over the submission, please check any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines.

The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention https://github.com/openjournals/joss-reviews/issues/4928 so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

We aim for the review process to be completed within about 4-6 weeks but please make a start well ahead of this as JOSS reviews are by their nature iterative and any early feedback you may be able to provide to the author will be very helpful in meeting this schedule.

ifthompson commented 2 years ago

Review checklist for @ifthompson

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

crvernon commented 1 year ago

📣 Mid-week rally!

👋 @pralitp and @ifthompson could you provide an update to how things are going? Also please let me know if you have any questions.

Keep up the good work!

ifthompson commented 1 year ago

I've worked my way through the checklist. Documentation is very nice and I was able to install and run successfully. Only thing I did not find was clear Community Guidelines for how users can contribute, report issues, and get support (I assume this is just raise an issue on the repository, so include a sentence saying that, or point to broader guidelines if they are elsewhere in the DTCC project).

A few other notes:

It might be helpful to include that tip for Windows users about the Unix-style line endings in the Installation section, in addition (or instead) of the Development section. Also, there may be a .gitattributes setting that tells Git not to convert line endings.

There is a typo in paper.md, "algorihtms" is misspelled in the Method and implementation first paragraph last sentence https://gitlab.com/dtcc-platform/dtcc-builder/-/blob/joss-2022/doc/paper-joss-2022/paper.md#method-and-implementation

alogg commented 1 year ago

@ifthompson Thanks, I have fixed these issues and pushed to the joss-2022 branch on GitLab:

  1. Typo in paper.md fixed

  2. Added .gitattributes with recommended configuration for Unix-style line endings

  3. Added documentation on how to contribute in contributing.md:

Contributing

Contributions to DTCC Builder are welcome and may come in the form of bug reports, feature requests, documentation, or code.

To contribute, register a GitLab account and post an issue detailing your bug report, feature request or idea for writing documentation or code on the project page for DTCC Builder.

Make sure to also check out the developer notes and note that any source code contributions need to be made open-source under the MIT License.

ifthompson commented 1 year ago

@alogg Great! I think that completes my checklist. @crvernon let me know if anything else is required on my end.

crvernon commented 1 year ago

👋 @pralitp could you provide an update to how things are going? Also please let me know if you have any questions.

Thanks @ifthompson you are all good for now!

crvernon commented 1 year ago

👋 @pralitp could you provide an update to how things are going? Also please let me know if you have any questions.

alogg commented 1 year ago

@crvernon Are we waiting for something? Anything we need to do on our end?

crvernon commented 1 year ago

@alogg no, you are all good. I'll check with our second reviewer again and get you some feedback ASAP. Thanks!

vbassn commented 1 year ago

@crvernon Sorry to bother you again, but I have finished reviewing for JoSS and I remember reading the soft "deadline" for reviewers is 6 weeks. I think we are way past that, think we could have it assigned to someone else if the second reviewer is unresponsive? PS: For some reason I cannot mention "pralitp"

crvernon commented 1 year ago

@vbassn I'll work on getting us a review that can turn this around quickly. Best.

crvernon commented 1 year ago

:wave: Hello @ipadjen - are you able to help with reviewing this for JOSS? Thanks!

ipadjen commented 1 year ago

Hello! Sure, I'll try to be swift

crvernon commented 1 year ago

Thanks @ipadjen!

crvernon commented 1 year ago

@editorialbot add @ipadjen as reviewer

editorialbot commented 1 year ago

@ipadjen added to the reviewers list!

crvernon commented 1 year ago

@editorialbot remove @pralitp as reviewer

editorialbot commented 1 year ago

@pralitp removed from the reviewers list!

crvernon commented 1 year ago

@ipadjen you can generate your checklist by commenting the following

@editorialbot generate my checklist

ipadjen commented 1 year ago

Review checklist for @ipadjen

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

crvernon commented 1 year ago

:mega: Mid-week Rally! :mega:

Looks like @ifthompson and @alogg are good to go on that side of the review. Nice work!

@ipadjen it also looks like you are making good progress! Can you post a general update here when you have a moment? Thanks!

ipadjen commented 1 year ago

Hello! I checked the repository and I have a few questions both for the authors and the editor.

@crvernon one checklist point states: 'Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.' My question: Is the list of dependencies necessary if the program is meant to be run through a docker container?

I initially tried using the default branch ('main') but that one did not work -- the docker container would not build. The branch 'joss-2022' was the one that worked well. I installed the repository using documentation and ran the example -- everything went fine. Afterwards, I tested the code with two of my own datasets (not in Sweden) and everything seems to be running fine -- the documentation is clear, it is straightforward to use the code and I have not had robustness issues.

With that in mind, I would like to address a few key points that I found relevant.

First, a few general notes:

Finally, here are my questions and remarks about the two main functionalities:

poly_with_holes

ipadjen commented 1 year ago

Also, once I get the confirmation that the new repo (on GitHub) should be used, I can open up issues referring to the review. We can then continue the discussion in respective threads, as asked in the first post of the review thread.

vbassn commented 1 year ago

@ipadjen thanks for your thorough review! @alogg and myself will get back with some answers!

anderslogg commented 1 year ago

@ipadjen Here's some initial response to some of your comments (more to follow).

Regarding the repo, yes we have moved to GitHub since we submitted the paper. So please use the GitHub repo for posting issues. The branch name is still joss-2022. Many things are different in the develop branch, in particular the addition of a Python interface (in progress).

Regarding the spelling of your name, we have this in the .bib file: Pa{\dj}en Is that not correct? We tried hard to get it right. :-)

Comments that remain to be handled:

  1. .obj export broken
  2. Requirements for point cloud classification
  3. Holes not handled
  4. Examples of using the meshes for simulation
ipadjen commented 1 year ago

Great! There, I opened up issues.

Yeah, about the name, I keep having this issue with a non-standard character. It is correct, but you also need an additional package in latex to make it work. I started writing 'd' instead, it's just easier.

anderslogg commented 1 year ago

@ipadjen One of the issues (note on point cloud classification) fixed and also the spelling (Pa{\dj}en --> Paden).

crvernon commented 1 year ago

@ipadjen was your earlier question regarding dependencies resolved or does this still need to be addressed?

crvernon commented 1 year ago

@alogg since you have changed over to GitHub, I will need to adjust the parent repository mentioned in this review thread. Let me see about that and I'll let you know when it's done. Please post a link to your new GitHub repo here.

ipadjen commented 1 year ago

@crvernon I am still wondering whether the list of all dependencies is necessary or not. The checklist item explicitly states its necessity, although I see little benefit to it with this repo. It's all in there in the docker anyway. What do you think?

crvernon commented 1 year ago

I agree that the containerization and accompanying files cover this @ipadjen. So I would say that this requirement has been satisfied. Thanks!

crvernon commented 1 year ago

:wave: @alogg please post the link to your new GitHub repo here so I can change the source for this thread.

@ipadjen and @alogg will you post updates to how things are going here? Thanks!

ipadjen commented 1 year ago

Hi @crvernon! I kindly asked authors to provide proof of their functional claims which I cannot confirm by simply running the code (e.g. show the results in the paper or somewhere in the repo). More here: https://github.com/dtcc-platform/dtcc-builder/issues/14.

Another issue that is currently open (https://github.com/dtcc-platform/dtcc-builder/issues/12) is not publication critical.

crvernon commented 1 year ago

Thanks @ipadjen !

anderslogg commented 1 year ago

@crvernon Link to new GitHub repo: https://github.com/dtcc-platform/dtcc-builder (branch joss-2022)

crvernon commented 1 year ago

@editorialbot set https://github.com/dtcc-platform/dtcc-builder as repository

editorialbot commented 1 year ago

Done! repository is now https://github.com/dtcc-platform/dtcc-builder

crvernon commented 1 year ago

@editorialbot set joss-2022 as branch

editorialbot commented 1 year ago

Done! branch is now joss-2022

crvernon 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.2148/benv.46.4.547 is OK
- 10.21105/joss.02866 is OK
- 10.1186/s40965-019-0064-0 is OK

MISSING DOIs

- 10.3389/fbuil.2022.899332 may be a valid DOI for title: Towards Automatic Reconstruction of 3D City Models Tailored for Urban Flow Simulations
- 10.1007/bfb0014497 may be a valid DOI for title: Triangle: Engineering a 2D Quality Mesh Generator and Delaunay Triangulator

INVALID DOIs

- None
crvernon commented 1 year ago

🚨 MIDWEEK RALLY 🚨

@ipadjen and @alogg - Could you please provide an update to how things are going here? As always, let me know if you have any questions!

Have a great day!

anderslogg commented 1 year ago

@crvernon @ipadjen

I think we're only waiting for fixing this issue: https://github.com/dtcc-platform/dtcc-builder/issues/14

I have it fixed already locally but haven't had time to push it and add to the docs due to other pressing matters (teaching). Will fix soon...

image

anderslogg commented 1 year ago

@crvernon @ipadjen I think I have fixed the final issue now!

crvernon commented 1 year ago

Great @anderslogg ! @ipadjen let me know when you are able to check your last box and we will move on to the next step in the process.

ipadjen commented 1 year ago

Yes, great! Simulation demo is a very nice touch, it would have already been enough with the visualisation of the results.

That concludes everything from my side. I hope I wasn't too annoying, but I think the story is nicely wrapped up now.

crvernon commented 1 year ago

@editorialbot check references