-
-
Notifications
You must be signed in to change notification settings - Fork 38
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
[REVIEW]: FEM_2D: A Rust Package for 2D Finite Element Method Computations with Extensive Support for *hp*-refinement #4172
Comments
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @jeremylt, @YohannDudouit it looks like you're currently assigned to review this paper 🎉. Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post. ⭐ Important ⭐ If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿 To fix this do the following two things:
For a list of things I can do to help you, just type:
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
|
Wordcount for |
|
|
Hi @jeremylt @YohannDudouit 👋 Welcome to JOSS and thanks for agreeing to review! The comments from @whedon above outline the review process, which takes place in this thread (possibly with issues filed in the FEM_2D repository). I'll be watching this thread if you have any questions. 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 this issue 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 reviews to be completed within a month or so. Please let me know if you require some more time. We can also use Whedon (our bot) to set automatic reminders if you know you'll be away for a known period of time. Please feel free to ping me (@jedbrown) if you have any questions/concerns. |
@jedbrown, could you resend the invite? I waited too long before clicking the link. |
Whedon retired young so we now ask |
Review checklist for @jeremyltConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
Review checklist for @YohannDudouitConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
I opened the issue jeremiah-corrado/fem_2d#2 containing a number of observations and suggestions. |
Since @YohannDudouit made a pretty comprehensive review on the overall structure and I agree with many of his points, I focused on creating a few issues highlighting some more Rust specific technical aspects: |
Thank you both for the detailed feedback! It's really nice to get an outside perspective on the design and documentation. I am currently in prep-mode for my thesis defense, but I should be able to start addressing these issues within a couple of weeks. In the meantime, I have a few questions and notes about the suggestions you made:
Thanks again for all the excellent feedback! |
I'll just respond to the question about scope. There is a limitless number of extensions and improvements for any nontrivial software so you have to draw the line somewhere. I think you want to try hard to make the architecture ready to receive contributions. If the new thing looks like implementing a trait or adding an enum variant, it's tractable to many more people than if it requires refactoring deeper assumptions. Such modularity can easily go overboard as well, but if you think about desired extensions and put yourself in the shoes of someone who might be interested in implementing, you should be able to identify what is "ready" and what is "theoretically, one could refactor the code to add". |
That all makes sense for your codebase at this moment in time - at the very least I'd recommend you document for your future self why those unwraps are valid. I still wouldn't do this though, personally. You are tightly coupling these pieces of code with implicit assumptions - specifically you are assuming that there is only one way for this function call to fail and that the constructor and/or specific usage for this object prevents this one failure mode. You are now responsible for maintaining these (undocumented!) assumptions through any changes to the function or the object. Why not just let the error handler keep track of this for you and let it give you a nice error message if future design decisions push you into an error case you hadn't originally anticipated? Unless you identify specific performance bottlenecks, I personally find it easier for future development to make use of the ergonomic language features and let the compiler optimize out the parts you don't need. I tend to assume that the error handling is free/nearly free until I profile that it is a bottleneck. Maybe I'm just a bit paranoid/defensive in my coding, but I tend to make everything but the simplest getters or helper functions return a Ultimately your call on what conventions you adopt here. I think it saves a ton of future headache to just use |
Hello all, Just a quick update to summarize some recent progress:
Thanks for all the feedback and discussion so far! |
That's great to hear. Thanks for the status update. |
@editorialbot generate pdf |
My issues have been addressed. One minor point - I suspect 'galerking sampling' on page 6, footnote 1 should be 'Galerkin Sampling' |
@YohannDudouit Could you give us an update as to whether your questions have been sufficiently addressed and the remaining items on your checklist? |
@jedbrown We may want to discuss specific points to see if they meet the JOSS criteria, in particular "Substantial scholarly effort", "Functionality", "Performance", "Functionality documentation", "A statement of need", and "State of the field". If I find this work overall interesting, I have some concerns though. The purpose of this library and the targeted audience is not very clear to me. The paper introduction claims "benefits" of the RBS approach without clearly stating what these are. The "Statement of need" opens with "Efficiently computing FEM solutions" and mentions the library Deal.II as a point of comparison, however it would be good to clarify what "efficient" means in this context and what are the aspects of Deal.II on which to compare (performance?). The main advantage over Deal.II seems to be "The succinctness of the continuity enforcement algorithm removes much of the difficulty of implementing new features. This is a major barrier to entry for contributing to larger and more complex packages."; I don't know Deal.II very well, but it seems suspicious to me that implementing new features usually requires a good understanding of continuity enforcement algorithms, for instance, in the MFEM library users very rarely have to be aware of these algorithms to implement new features. After investigating the implementation I also have strong doubts about the performance and therefore the capacity to "Efficiently comput[e] FEM solutions". I also find the documentation minimalist, the documentation of traits, struct, and functions rarely exceed a few lines. The main page is also outdated, the
This brings me to some of the issues I have been trying to bring attention here. One of the main advantages of the Rust language is to allow zero-cost high-level abstraction, therefore having interfaces close to the mathematical concepts while hiding implementation details. I think the |
Thanks @YohannDudouit for your comprehensive review. @jeremiah-corrado how are you doing? From the perspective of JOSS, we are looking for accuracy. So if you don't want to make major changes to the library, you can adjust the abstract to make clear that it's a library for linear As an aside, I notice that the quadrature interface handles only weights, leaving the points to the user to index manually. This |
Hi @jeremiah-corrado 👋. I just wanted to check in about your plans. To be clear, you don't need to make major changes to the code in order to publish in JOSS, but the paper and docs need to accurately state what the software is now, not what you imagine it can become. Of course you're welcome to act on the review in the form of major code changes. |
@jeremiah-corrado - I'm checking on the warning above and proofreading your paper, and will let you know next steps soon |
@jeremiah-corrado - I've suggested some changes for language and to fix the warnings in jeremiah-corrado/fem_2d#12. You also might want to define/explain h and p in the fourth paragraph of the paper (or before that). |
@jedbrown - As a note, this paper now seems far too long for a JOSS paper, and I think that a lot of it probably should be in the documentation, but its not worth changing at this point. I don't think it was so long when it was first submitted, so I assume additions were made during the review process. If I'm wrong and it started out this long, sorry. But if I'm right, please consider this in future reviews, where some of the details in the paper be in the documentation instead. |
@editorialbot check repository |
|
Wordcount for |
Thanks @danielskatz, I merged your PR and added a brief description of h- and p-refinement to paragraph four. |
@editorialbot recommend-accept |
|
|
👋 @openjournals/csism-eics, this paper is ready to be accepted and published. Check final proof 👉📄 Download article If the paper PDF and the deposit XML files look good in openjournals/joss-papers#4163, then you can now move forward with accepting the submission by compiling again with the command |
@editorialbot accept |
|
Ensure proper citation by uploading a plain text CITATION.cff file to the default branch of your repository. If using GitHub, a Cite this repository menu will appear in the About section, containing both APA and BibTeX formats. When exported to Zotero using a browser plugin, Zotero will automatically create an entry using the information contained in the .cff file. You can copy the contents for your CITATION.cff file here: CITATION.cff
If the repository is not hosted on GitHub, a .cff file can still be uploaded to set your preferred citation. Users will be able to manually copy and paste the citation. |
🐦🐦🐦 👉 Tweet for this paper 👈 🐦🐦🐦 |
🐘🐘🐘 👉 Toot for this paper 👈 🐘🐘🐘 |
🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨 Here's what you must now do:
Any issues? Notify your editorial technical team... |
Congratulations to @jeremiah-corrado (Jeremiah Corrado) and co-authors on your publication!! And thanks to @jeremylt and @YohannDudouit for reviewing, and to @jedbrown for editing! (I see that the DOI isn't yet resolving, so I'll keep this issue open until it does, which hopefully should be in the next few hours at the latest) |
The DOI is now resolving! |
🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉 If you would like to include a link to your paper from your README use the following code snippets:
This is how it will look in your documentation: We need your help! The Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:
|
Submitting author: @jeremiah-corrado (Jeremiah Corrado)
Repository: https://github.com/jeremiah-corrado/fem_2d
Branch with paper.md (empty if default branch):
Version: 0.2.2
Editor: @jedbrown
Reviewers: @jeremylt, @YohannDudouit
Archive: 10.5281/zenodo.7849878
Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.
Status
Status badge code:
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
@jeremylt & @YohannDudouit, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @jedbrown 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 ✨
The text was updated successfully, but these errors were encountered: