Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JOSS] Review comments #181

Closed
PieterjanRobbe opened this issue Sep 9, 2024 · 2 comments
Closed

[JOSS] Review comments #181

PieterjanRobbe opened this issue Sep 9, 2024 · 2 comments

Comments

@PieterjanRobbe
Copy link

Hi @DanielVandH, thank you so much for such an exciting Julia package!

I just finished my JOSS review at openjournals/joss-reviews#7174.

I think you did an amazing job with this package. The software is well-scoped, with a clear purpose to generate triangulations and tessellations in Julia in an efficient manner. I think it fills a clear gap described in the statement of need, and I appreciate the time and effort you took comparing your code to other existing (Julia or non-Julia) codes with similar purposes. The plotting capabilities (in Makie.jl) look great too!

I think the documentation and extensive tutorials sets this package apart from the competition. The topic of this package is somewhat far from my field of expertise, but I felt like I learned a lot by just browsing the tutorial section. Thank you!

I just have some (very minor) comments / suggestions / questions that, in my opinion, would make the package even better:

Manuscript

  • I would suggest swapping paragraphs one and two, or rephrasing the opening sentence. I think the definitions of Delaunay triangulation and Voronoi tessellation are on point, but I feel like they should come later in the introduction, perhaps after introducing the package and its core features.
  • DelaunayTriangulation.jl is the most feature-rich ... I think this is somewhat of a bold claim, especially since there's no reference confirming this. I feel like you do address this appropriately in the section on similar packages in the README, so perhaps a reference to that section is sufficient here?

README

  • When running the example code in the README.md file, I get some seemingly strange results for the Power Diagram plot. There appears to be only one point. I'm not sure if this is intentional?
  • Also, on my laptop, the lines with the plotting commands to not appear fully on screen. Perhaps these triplot! and voronoiplot! commands could go on a new line?
  • Finally, I noticed there is a CONTRIBUTING.md file, but perhaps you could also refer to that file somewhere in the README.

Thanks!

Screenshot 2024-09-09 at 2 56 14 PM
@DanielVandH
Copy link
Member

DanielVandH commented Sep 10, 2024

Hi @PieterjanRobbe, thanks very much for your review and your kind words! I very much appreciate it and your time. I'm glad you recognise the effort that I've put in. I've addressed all your comments below, but to summarise I rearranged the order of the summary paragraphs, weakened the language used for comparing my package to other software, and I've cleaned up the README. I'll generate a new PDF over at the review issue that you can read to see the differences.

I would suggest swapping paragraphs one and two, or rephrasing the opening sentence. I think the definitions of Delaunay triangulation and Voronoi tessellation are on point, but I feel like they should come later in the introduction, perhaps after introducing the package and its core features.

This is a good point. I've made some adjustments to these paragraphs and adjusted their order. I also now mention that the package supports weighted triangulations and power diagrams since those were added this week.

DelaunayTriangulation.jl is the most feature-rich ... I think this is somewhat of a bold claim, especially since there's no reference confirming this. I feel like you do address this appropriately in the section on similar packages in the README, so perhaps a reference to that section is sufficient here?

You're right, this is a strong claim. I've made the wording less bold here and just claim to features not present in most of these other packages. I also now refer to other existing Julia packages, and link to the corresponding section of the README.

When running the example code in the README.md file, I get some seemingly strange results for the Power Diagram plot. There appears to be only one point. I'm not sure if this is intentional?

Power diagrams with random weights are a fickle thing. It is easy to get cases like yours (not because of any bugs, but just because the random weight happens to make one cell completely dominate). What I've done now is put an explicit rng object in the example using StableRNGs to at least guarantee a good looking power diagram. You can look at the new README already committed to see the difference.

Also, on my laptop, the lines with the plotting commands to not appear fully on screen. Perhaps these triplot! and voronoiplot! commands could go on a new line?

Thanks for noting that, I've changed it now to remove the space taken up by all the width = 600, height = 600 terms. I think this is better than just doing a new line - what do you think?

Finally, I noticed there is a CONTRIBUTING.md file, but perhaps you could also refer to that file somewhere in the README.

I refer to this already in the README right before the example. Is there a way I clearer way you want me to refer to it?

Thanks again.

@PieterjanRobbe
Copy link
Author

Hi @DanielVandH, thanks for the quick follow-up! All of these changes look good to me. I must have missed the reference to the CONTRIBUTING.md file, thanks for pointing this out! I'll close the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants