[REVIEW]: FieldCompare: A Python package for regression testing simulation results #4905
Comments
editorialbot
added
Python
review
TeX
Track: 7 (CSISM)
|
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: For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type: |
|
|
Wordcount for |
|
|
@idoby and @WilliamJamieson - Thanks for agreeing to review this submission. As you can see above, you each should use the command 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, reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention We aim for reviews to be completed within about 2-4 weeks. Please let me know if either of you require some more time. We can also use editorialbot (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 (@danielskatz) if you have any questions/concerns. Reminder from pre-review issue: @WilliamJamieson won't be available to work on this until the week of 14 Nov |
Review checklist for @idobyConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
Review checklist for @WilliamJamiesonConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
|
@WilliamJamieson - Can you comment on the items in your review that are not checked off? What is needed for you to check them to off? As stated above
|
|
👋 @idoby - how is your review coming along? Is anything blocking you from continuing? |
Sorry I did make a couple of issues:
|
|
Thanks - I'll add the tag in those. |
|
I guess this isn't going to work well, since these are in gitlab, not github - oh well... |
|
@dglaeser - Can you take a look at the issues @WilliamJamieson created? |
|
👋 @idoby - how is your review coming along? Is anything blocking you from continuing? |
|
Hi, sorry, life got in the way of this. I'm posting my concerns here because I think they're too broad for GH/GL issues. In short, I have concerns about the design of this package. While I'm not sure this falls strictly under one or more checklist items, I feel the design severely impairs the usability, maintainability and future extensibility of this package. My main concerns are:
Therefore, I think the authors should consider the following points:
I realize the above represents a major rewrite, but I sincerely believe this will improve your package and raise the chances that people (like me) will use it. |
|
👋 @WilliamJamieson and @dglaeser - please let us know your thoughts on these ☝️ comments |
|
Hi @WilliamJamieson, @idoby and @danielskatz, thanks a lot for your thorough review and the helpful comments! I think we'll need some time to adress them, but I'll add a few initial thoughts/responses on the comments by @idoby here (I responded to @WilliamJamieson in some of the open issues already, thanks at this point also for your suggestions!). First of all, thanks @idoby for the comments, they are highly appreciated! I think that they can help to converge to a solution that is best suited for users that want to interact with the library in different ways (since our own so far has been mainly via the CLI). However, I think it would be good if we can maybe converge on what the main contributions of this package may be. So far, I had the feeling that the main benefits it provides are for rather "high-level" use, but I'd be more than happy to improve usability for different use cases!
Good point, thanks! I am tempted to remove the logging facilities from the public API, the main purpose was for us to have a configurable way to control the output from the CLI onto the terminal or to the test reports. If we keep this exposed to the user, it would definitely be better to accept standard loggers. Thanks, I'll address this.
Wrapping of numpy was mainly done for internal use such that in the rest of the code we don't do any direct calls to numpy, but keep track of which things we actually need/use while having the opportunity to later move to a different package if required (although very unlikely) with only changing a single file.. Only the factory functions are exposed to the public API to create an instance of an I am not sure meshio is really "wrapped". We currently expose a simple reader function that takes a file and returns a container of fields. Meshio is used under the hood, but users should not notice or know about this? We simply use it under the hood because it provides reading functionality for many file formats of interest for us. But users never interact with meshio types, we always return our own container types. I think what's missing for your case is direct support for a meshio
As said, I think your comments really help to improve the programmatic usage scenarios, and that it maybe would be good if we can converge on what the main contributions of this package can be (e.g. for your use case)? Or maybe asked differently, do you think there is a valuable contribution to your use case at all? As far as I see it, we currently have a. High-level I/O mechanisms that yield our own container type interface for fields. Point (a) & (b) make up the CLI basically, which so far has been our own main use case. When skipping (b) and doing array comparisons (c) manually, one can also directly use numpy. So I think the main contributions so far are on the higher levels (and the CLI). I noticed that our mesh-field-transformations are currently not exposed in the public API. They wrappers around @danielskatz, is there a time limit for addressing the points? I feel that this will involve quite some rewriting. But improving the API as suggested by @idoby and adding a proper documentation as suggested by @WilliamJamieson will definitely improve this package a lot. |
|
Thanks for replying to my comments. My thoughts:
In my opinion, the library should should either not use logging at all, or it should let the user pass in a standard logger. I'd prefer the library not to log anything unless a debug mode is enabled explicitly. I don't really see why a library like this should throw lines into my output. I'd prefer a nice function to nicely format the data structures returned by the lib, instead, that I can call whenever I want, send to my server, etc.
Wrapping numpy is not a big issue, but I'd prefer public helper functions to be under fieldcompare.utils.numpy or something, to make their purpose clear. I don't think there's a need to hedge against numpy getting replaced, but I don't mind if you do. It's not a big issue. My main point was about meshio.
I understand. My issue with this was that you were assuming my data pipeline begins by reading a single file my meshio, so might as well let fieldcompare drive meshio. But, this assumption makes many use cases I want to use, impossible. In any case, the difference between (pseudocode) mesh = meshio.load(...)
fieldcompare.sort(mesh).compare()and fieldcompare.load(filepath).sort().compare()is a minimal burden to your user, but in return, this enables fieldcompare to integrate into existing data pipelines.
I see. In that case I recommend to add a few words about this in the paper, but it's not as big of a deal as I thought. Since you're importing meshio from Python and not your own baked-in version. So that only requires patching meshio in site-packages, which you'd have to do anyway to add a new cell type. So it's not that bad. I still think if you avoid wrapping meshio then that extra code can live outside of a meshio fork, in the user's own project, and also allow the use of other mesh loaders, like trimesh, but it's not as bad as I originally thought. In summary, I think avoiding driving the I/O (and specifically meshio) makes fieldcompare much more useful as a library than it currently is, although I agree it doesn't matter for CLI usage. However, I don't think the CLI is the primary contribution here, even if considering CI/CD use cases, because again, your user is likely to have their own data pipeline, use additional libraries, and they might not map data to files 1:1.
I agree that mesh sorting is a great contribution, and one of the two main contributions in your package. I even think I might want to use it without the comparison logic, e.g., to sort the mesh for use in loss functions. So I think fieldcompare could benefit from more flexibility across the board. I
In the pipeline-like API, the user "deactivates" sorting in the pipeline API by not calling it :) .
Good point. This can probably be done in pandas alone and FC isn't needed in that case, though it could be nice if it was possible to extract a
For me, the main contributions are 1) mesh field handling/sorting/transformation and 2) comparison and reporting logic. Both will probably be useful for my use case, but I don't think one interface should be favored over the others. Please consider making all three interfaces I described above equally usable. I think this also addresses the rest of your comments? I just don't see the CLI as being more or less important than the programmatic interface, and believe the contribution here is greater than just a neat CLI tool. Either way, I will not block the review on these grounds, if you consider the CLI to be the main use case and the library itself not worth investing in, I respect that decision. @danielskatz Please consider adding something about design and extensibility to the checklist. The JOSS docs do mention these words, but it would be nice to have them explicitly in the checklist IMO, for anyone who wants to conduct reviews with these considerations in mind. |
If you want to suggest changes for JOSS, doing this by an issue in https://github.com/openjournals/joss/issues is probably the best way. If you suggest specific language you would like, that will give us a starting point for discussion. |
|
@idoby, thanks a lot again for your detailed message, it helped me to understand some of your points much better. I am happy you bring up usage scenarios that we had not in mind, and I really appreciate the valuable comments/ideas on how to make them possible.
As said, our focus was - above all - the CLI, but I would be happy to invest time and effort to improve the API by addressing the points you raised. @danielskatz, do we have a limit on when we have to have this finished? |
|
there's not a strict limit - How long do you think this will take? |
|
It is going to be very difficult to address this before Christmas, I think. I'll try, but as usual many things pile up towards the end of the year... January is usually a bit more relaxed, so I am pretty confident that I can propose a version with the points addressed until the end of January (maybe earlier). Would that be too late? |
|
I think that's ok - let's try it and see |
|
@editorialbot recommend-accept |
|
|
@dglaeser - please check the proof version that is being generated now, to make sure nothing was incorrectly changed in my minor edits, and let me know |
|
|
👋 @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#3926, then you can now move forward with accepting the submission by compiling again with the command |
|
@danielskatz, I stumbled across three things, which I think were introduced in one of your merge requests. I edited them here: https://gitlab.com/dglaeser/fieldcompare/-/merge_requests/195, would you mind double-checking? I noticed after my modifications in !195 that you had just introduced the changes related to putting the punctuation inside the parentheses. I wasn't aware that this is the rule, but I also couldn't verify that it is with a (quick) google search. The top matches of my search suggest the opposite: https://blog.apastyle.org/apastyle/2013/03/punctuation-junction-periods-and-parentheses.html But if this is the correct way to go, I can simply kick out the two commits again. I guess the commit fixing the newline should be fine. |
|
the punctuation change I made is correct for most US English, which I think is what is being used here. I don't think the newline matters either way, as both should generate the same PDF, I think |
|
sorry, you are right - the punctuation change I made was wrong. |
|
ok, so I merge !195? Everything else looked fine to me. I noticed the newline while reading the proof version of the pdf, this is what I get:
|
|
I guess so - I'm not sure why that is happening, but let's try to fix it |
|
I merged it, the .pdf looks fine to me now. |
|
@editorialbot recommend-accept Let's try an official preview again. Can you again check this after it completes, and verify that it look good to you? |
|
|
|
👋 @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#3927, then you can now move forward with accepting the submission by compiling again with the command |
|
@danielskatz looks good to me, thanks a lot! |
|
@editorialbot accept |
|
|
🐦🐦🐦 👉 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... |
|
thanks a lot for your help @danielskatz, @idoby, @WilliamJamieson ! |
|
@dglaeser Sure thing, congrats! 🥳 |
|
Congratulations to @dglaeser (Dennis Gläser) and co-authors!! And thanks to @idoby and @WilliamJamieson for reviewing! |
|
🎉🎉🎉 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: @dglaeser (Dennis Gläser)
Repository: https://gitlab.com/dglaeser/fieldcompare
Branch with paper.md (empty if default branch): feature/paper
Version: 0.1.3
Editor: @danielskatz
Reviewers: @idoby, @WilliamJamieson
Archive: 10.5281/zenodo.7588449
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
@idoby & @WilliamJamieson, 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:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @danielskatz 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 @idoby
📝 Checklist for @WilliamJamieson
The text was updated successfully, but these errors were encountered: