-
-
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]: Diart: A Python Library for Real-Time Speaker Diarization #5266
Comments
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 |
|
Review checklist for @mensisaConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
(pinging to say sorry i am so delayed, working on a time-sensitive writing project and this is on my list for when i finish that this week) |
Hi @sneakers-the-rat, any updates on the review? Please feel free to let us know if you need any help. Thanks again for your time. |
Sorry, have been having a rough few months. just getting back up to speed with my work responsibilities, and I think i'll be able to get to this next week. Sorry for the extreme delay |
Again extremely sorry, working on this now. |
Review checklist for @sneakers-the-ratConflict of interest
Code of Conduct
General checks
General comments: One minor nitpicky thing I would say here is that it would be good to get the images and paper pdf out of the root of the repo as it makes the package feel less tidy than it actually is! other than that looks good!!! Functionality
General comments: Code runs! joy! DocumentationCollected docs issue here - juanmc2005/diart#176
Additional thoughts on docs:
Software paper
SummaryThanks for the very lovely package! Code structure is great, clearly written and commented, and all the cli entrypoints run out of the box with very little fuss (aside from that described in the README for installation, but thats totally fine). I get the feeling that this is an extension of a prior package, so I'm not quite sure what the expectations for documentation and testing should be - the code itself is readable, the examples seem to cover major functionality, and there are some docstrings, but there isn't anything you would call "API documentation" here. The most glaring thing missing here is a test suite. The package seems to run fine from what I could tell, but I am unable to evaluate the accuracy of the results because I don't know that the intermediate functions/methods are well tested. I can sympathize with the author in not wanting to write them (again, particularly if this is an extension package to a well-tested prior package), but requiring tests is about more than just making sure the code stays working, but that it is maintainable and possible to contribute to for the overall health of the scientific programming ecosystem - if the author gets busy with life and has to drop maintenance, would anyone else be able to pick it up? will anyone be able to run this in 5 years? tests make that question a lot easier to answer. I'm not sure how the other reviewer passed this checkmark because they're not there! It doesn't necessarily bear on the review process at JOSS, but I do feel like I need to say that I don't love needing to create an account on huggingface.co to test the package, and I am skeptical about making packages dependent on some cloud service existing for free indefinitely (i am aware GitHub is also a platform, but hopefully git makes redundant enough copies of things, and archiveteam is on that), so i would love to see the authors host the model weights elsewhere as another option, even on someplace like zenodo would be great. The models are quite small and would even fit in this repository, not sure why they aren't. Again this doesn't bear on this review process at all. That said, there is a lot to love here (one minor ux thing, it's rare to see progress bars for multiprocessing tasks done well. The model provisioning worked smoothly once I installed the huggingface-cli, which was lovely (i usually hate that part). I also loved the live plot of the results when I was using my mic). Overall this is a well executed tool that does exactly what it describes. I wish I had more time to spend with the code (i know this will read as ironic given how long it took me to actually get to the review), as I usually like to hunt around for code improvements that the author can make to provide whatever training or tips i can, but it's also relatively clear they don't need it as this package is very well written. I don't believe in reviewing-as-gatekeeping, so every review is a "pass" from me. My comments here, and boxes left unchecked above are to indicate to the authors where things could be improved and to the editor if they need to uphold the checklist. Issues Raised
Pull requests |
stalled on juanmc2005/diart#158 - can't run program |
@juanmc2005 @Fei-Tao – it looks like @sneakers-the-rat is blocked by juanmc2005/diart#158 – could you agree a plan to move this forward please? |
I am very sorry for the even more extreme delay than last time. Still on my TODO but keeps getting shunted down after prepping for a conference. Ill be releasing a package early next week after which ill make time BC this has.gone on too long already! |
Hi @arfon, thanks for your follow-up with this submission. Since sneakers-the-rat has plan to finish reviewing this submission, can we wait for his response? @sneakers-the-rat No worry. That's understandable. Thanks for your time in reviewing this submission. I'm looking forward to hearing your comments and suggestions. Please feel free to let us know if you need any help. |
Of course! Just wanted to prompt to see what is happening here. |
@arfon Great. Thanks for your prompt response! |
@editorialbot set 10.5281/zenodo.12510278 as archive |
Done! archive is now 10.5281/zenodo.12510278 |
@editorialbot check references |
|
@editorialbot set v0.9.1 as version |
Done! version is now v0.9.1 |
@editorialbot recommend-accept |
|
|
👋 @openjournals/dsais-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#5554, then you can now move forward with accepting the submission by compiling again with the command |
Hi @crvernon, this submission looks good to me now. Can you take it from here? Thanks for your time. |
🔍 checking out the following:
|
👋 @juanmc2005 - I just have the following few things I need you to address before I move to accept this publication:
Thanks! |
@juanmc2005 just pinging you again on the above. |
@crvernon I just addressed the changes you requested |
@editorialbot generate pdf |
@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. |
🐘🐘🐘 👉 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 on your new publication @juanmc2005! Many thanks to @Fei-Tao for editing and @sneakers-the-rat and @mensisa for your time, hard work, and expertise!! JOSS wouldn't be able to function nor succeed without your efforts. Please consider becoming a reviewer for JOSS if you are not already: https://reviewers.joss.theoj.org/join |
🎉🎉🎉 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:
|
Thank you all! |
Submitting author: @juanmc2005 (Juan Manuel Coria)
Repository: https://github.com/juanmc2005/StreamingSpeakerDiarization
Branch with paper.md (empty if default branch): joss
Version: v0.9.1
Editor: @Fei-Tao
Reviewers: @sneakers-the-rat, @mensisa
Archive: 10.5281/zenodo.12510278
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
@sneakers-the-rat & @mensisa, 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 @Fei-Tao 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 @mensisa
📝 Checklist for @sneakers-the-rat
The text was updated successfully, but these errors were encountered: