search

openjournals / joss-reviews

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

[REVIEW]: PIVC: A C/C++ Program for Particle Image Velocimetry Vector Computation #3736

Closed whedon closed 1 year ago

whedon commented 2 years ago

Submitting author: !--author-handle-->@fibreglass2<!--end-author-handle-- (Kadeem) Repository: https://gitlab.com/fibreglass/pivc Branch with paper.md (empty if default branch): Version: v1.2-b Editor: !--editor-->@Kevin-Mattheus-Moerman<!--end-editor-- Reviewers: @clarka34, @quynhneo Archive: 10.5281/zenodo.7556040

:warning: JOSS reduced service mode :warning:

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

Status badge code:

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

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

@timdewhirst & @clarka34 & @quynhneo, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @Kevin-Mattheus-Moerman 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 ✨

Review checklist for @timdewhirst

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @clarka34

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @quynhneo

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 2 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @timdewhirst, @clarka34, @quynhneo it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

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.

:star: Important :star:

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:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf
whedon commented 2 years ago

Wordcount for paper.md is 938

whedon commented 2 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.03 s (400.7 files/s, 47586.5 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C++                              4             72             78            794
TeX                              1             32              0            260
Markdown                         3             49              0            176
C/C++ Header                     4             15              1             59
make                             1              1              0              7
-------------------------------------------------------------------------------
SUM:                            13            169             79           1296
-------------------------------------------------------------------------------

Statistical information for the repository 'b544fa72933c272acc8be899' was
gathered on 2021/09/18.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
fibreglass                      14          1387            368          100.00

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
fibreglass                 1019           73.5          0.0                7.75
whedon commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1007/s003489900085 is OK
- 10.1007/s00348-016-2173-1 is OK
- 10.5334/jors.334 is OK
- 10.5334/jors.bl is OK
- 10.1088/0169-5983/47/3/035509 is OK
- 10.1016/j.ijthermalsci.2013.11.001 is OK
- 10.1016/j.ijheatfluidflow.2013.12.005 is OK
- 10.1016/j.ijthermalsci.2014.11.026 is OK
- 10.1063/1.1375144 is OK
- 10.1016/j.expthermflusci.2020.110286 is OK
- 10.1016/j.est.2019.100825 is OK
- 10.1016/j.ijheatfluidflow.2021.108839 is OK
- 10.1016/j.ijheatfluidflow.2008.01.003 is OK
- 10.1063/1.2185709 is OK
- 10.1007/s00231-005-0072-8 is OK
- 10.1063/1.3054153 is OK
- 10.1088/0957-0233/18/1/012 is OK
- 10.1017/s0022112006003892 is OK
- 10.1016/j.ijthermalsci.2015.05.003 is OK
- 10.1016/j.jweia.2021.104605 is OK
- 10.1002/we.1895 is OK
- 10.1007/s10236-008-0132-y is OK
- 10.1016/j.solener.2010.02.008 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon 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:

Kevin-Mattheus-Moerman commented 2 years ago

Reproducing @timdewhirst 's comments from the pre-review issue (https://github.com/openjournals/joss-reviews/issues/3603):

I've raised a few issues; links are at the bottom of this comment.

As I understand it there were two main questions around the code: does the use of OpenMP help take advantage of multicore processors, and is the code well structured and a suitable foundation for a collaborative project implementing various PIV processing algorithms?

On OpenMP: this appears to work as expected.

On code structure: this is my main area of concern; the code need work to follow best practices in structure, legibility, efficiency and testing. Very little C++ is used, and this is a shame; modern C++ has powerful features for generic and functional style programming which would allow the code to be cleaner, more legible, and just as efficient. Currently, I don't believe it's in good enough shape to act as a foundation for significant collaborative development.

issues:

Kevin-Mattheus-Moerman commented 2 years ago

@timdewhirst, @clarka34, @quynhneo this is where the actual review takes place. Thank you all for you help! Please have a look at the information at the top of this issue and the check-boxes that will guide the review. Let me know if you have any questions.

Kevin-Mattheus-Moerman commented 2 years ago

@quynhneo to answer your query over at #3603, have a look at our review guidelines. Let me know if you have questions.

clarka34 commented 2 years ago

I have added some more issues:

At this stage, my main concerns are the lack of real-word examples for users that are not very familiar with PIV. Additionally, the documentation could be extended and improved to make it easier for future collaboration/use of the software. It would be difficult for a person who is not familiar with PIV to understand the grid spacing options and what to do with the result files.

Kevin-Mattheus-Moerman commented 2 years ago

@fibreglass2 as you may have seen there are a number of issues open that were raised by the reviewers. :point_up:. Can you start to address these? Let me know if you have any questions.

clarka34 commented 2 years ago

Sorry, my phone has had a mind of its own this morning. It seems like I have unassigned @timdewhirst and closed this. I reopened but could not figure out how to reassign. @Kevin-Mattheus-Moerman can you do this?

Kevin-Mattheus-Moerman commented 2 years ago

@openjournals/dev can you check if I need to fix anything? ☝️

Kevin-Mattheus-Moerman commented 2 years ago

@clarka34 no worries we'll sort it out

clarka34 commented 2 years ago

@Kevin-Mattheus-Moerman thank you! again, I am sorry about this...

fibreglass2 commented 2 years ago

@fibreglass2 as you may have seen there are a number of issues open that were raised by the reviewers. point_up. Can you start to address these? Let me know if you have any questions.

Thanks to the reviewers for the detailed feedback. I will be working to investigate and address issues raised.

whedon commented 2 years ago

:wave: @quynhneo, please update us on how your review is going (this is an automated reminder).

whedon commented 2 years ago

:wave: @timdewhirst, please update us on how your review is going (this is an automated reminder).

whedon commented 2 years ago

:wave: @clarka34, please update us on how your review is going (this is an automated reminder).

Kevin-Mattheus-Moerman commented 2 years ago

@fibreglass2 how are you getting on? Do you have any updates to report?

fibreglass2 commented 2 years ago

@fibreglass2 how are you getting on? Do you have any updates to report?

As shown on the repository's issue tracker, several issues have been raised by the reviewers. I have made updates to address a number of documentation issues. Some of the issues raised relate to adding features or changing major portions of the existing design. These feature related issues will take additional time to investigate and address.

Kevin-Mattheus-Moerman commented 2 years ago

@fibreglass2 let me know if there is anything I can help with or if we can resume review after you've addressed the reviewer comments. Thanks.

quynhneo commented 2 years ago

Sorry I didn't meet the suggested 6 weeks deadline for the review, but I will catch up soon.

fibreglass2 commented 2 years ago

@fibreglass2 let me know if there is anything I can help with or if we can resume review after you've addressed the reviewer comments. Thanks.

I'd like to address most to all of the reviewer comments, including ones to be added by @quynhneo. After that review can resume.

Kevin-Mattheus-Moerman commented 2 years ago

@fibreglass2 I hope you are getting on well. Let us know if you have any updates.

Kevin-Mattheus-Moerman commented 2 years ago

@fibreglass2 :wave: :point_up:

quynhneo commented 2 years ago

I am sorry guys, I ll catch up this weekend.

fibreglass2 commented 2 years ago

@Kevin-Mattheus-Moerman I hope you are getting on well. Let us know if you have any updates.

This weekend I will be addressing multiple issues raised by the previous reviewers. It might be good for @quynhneo to hold off on a full review until I release this update to save time and reduce duplication.

quynhneo commented 2 years ago

Sure thanks

On Fri, Dec 10, 2021 at 17:50 fibreglass2 @.***> wrote:

@Kevin-Mattheus-Moerman https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Kevin-2DMattheus-2DMoerman&d=DwMCaQ&c=slrrB7dE8n7gBJbeO0g-IQ&r=jQuWDziv5_zEIYYAA3r4Rw&m=tOJ5hOsttvLJDjW5HwxpMwWmXJPsTpETQqaHsR1iyAQswImxmLP5VggKbeMQ4zfV&s=IgWzE4DxKuLVww30DcTTxfJr58Niinaof9xqmdqViXc&e= I hope you are getting on well. Let us know if you have any updates.

This weekend I will be addressing multiple issues raised by the previous reviewers. It might be good for @quynhneo https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_quynhneo&d=DwMCaQ&c=slrrB7dE8n7gBJbeO0g-IQ&r=jQuWDziv5_zEIYYAA3r4Rw&m=tOJ5hOsttvLJDjW5HwxpMwWmXJPsTpETQqaHsR1iyAQswImxmLP5VggKbeMQ4zfV&s=BjsC6j_lGpcrUVZn5DnWUp0gWJ_D6wk5wn-onXgycu8&e= to hold off on a full review until I release this update to save time and reduce duplication.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openjournals_joss-2Dreviews_issues_3736-23issuecomment-2D991355830&d=DwMCaQ&c=slrrB7dE8n7gBJbeO0g-IQ&r=jQuWDziv5_zEIYYAA3r4Rw&m=tOJ5hOsttvLJDjW5HwxpMwWmXJPsTpETQqaHsR1iyAQswImxmLP5VggKbeMQ4zfV&s=HtxDf9o9aJdNua2vK0rW0c7UvwGJy19YetD1IsV-nkA&e=, or unsubscribe https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ACRAU7REG7TSAXOUJJQ5KITUQJ727ANCNFSM5EJRS76A&d=DwMCaQ&c=slrrB7dE8n7gBJbeO0g-IQ&r=jQuWDziv5_zEIYYAA3r4Rw&m=tOJ5hOsttvLJDjW5HwxpMwWmXJPsTpETQqaHsR1iyAQswImxmLP5VggKbeMQ4zfV&s=4vWa3nABv1NXyheirIdTUQhGOASssbO9sAV-uqZEKt8&e= . Triage notifications on the go with GitHub Mobile for iOS https://urldefense.proofpoint.com/v2/url?u=https-3A__apps.apple.com_app_apple-2Dstore_id1477376905-3Fct-3Dnotification-2Demail-26mt-3D8-26pt-3D524675&d=DwMCaQ&c=slrrB7dE8n7gBJbeO0g-IQ&r=jQuWDziv5_zEIYYAA3r4Rw&m=tOJ5hOsttvLJDjW5HwxpMwWmXJPsTpETQqaHsR1iyAQswImxmLP5VggKbeMQ4zfV&s=f2254hSo6aa6yKLhZG6xMkP656A6na4UU3Mz7fR87BI&e= or Android https://urldefense.proofpoint.com/v2/url?u=https-3A__play.google.com_store_apps_details-3Fid-3Dcom.github.android-26referrer-3Dutm-5Fcampaign-253Dnotification-2Demail-2526utm-5Fmedium-253Demail-2526utm-5Fsource-253Dgithub&d=DwMCaQ&c=slrrB7dE8n7gBJbeO0g-IQ&r=jQuWDziv5_zEIYYAA3r4Rw&m=tOJ5hOsttvLJDjW5HwxpMwWmXJPsTpETQqaHsR1iyAQswImxmLP5VggKbeMQ4zfV&s=JnTp37KMKthwQaEOK1JfjNcVhgO61IEiMS_LD37ov6o&e=.

-- Quynh Nguyen, Physics PhD Physics Department, New York University and Applied Math Laboratory, Courant Institute of Mathematical Sciences Address: Room 104, 251 Mercer Street, New York NY 10012 Phone : 1 (714) 902 7149 E-mail : @.***

fibreglass2 commented 2 years ago

Several points have been addressed in the newest update. However part of the newly implemented features, specifically the UI, is not cross compatible with Windows. Until I can get a fully working UI for Windows, the update in the repo is for GNU/Linux only. In the interest of this review, the source code is available for inspection in the repo.

fibreglass2 commented 2 years ago

As a follow up to my last comment, PIVC has just been updated. It is now fully working in Windows. Please see version 1.2, windows branch, of the repository. The documentation has been updated to reflect the changes. Thank you to all of the reviewers for their patience in this process.

Kevin-Mattheus-Moerman commented 2 years ago

@fibreglass2 does this mean the reviewers should pick this up again or should they hold off still?

fibreglass2 commented 2 years ago

@fibreglass2 does this mean the reviewers should pick this up again or should they hold off still?

Please continue the review.

Kevin-Mattheus-Moerman commented 2 years ago

@timdewhirst, @clarka34, @quynhneo the author has made several updates, could you please resume the review at this point or update us on the process? Thanks a lot again for your continued help!!

quynhneo commented 2 years ago

hi there, I raised an issue https://gitlab.com/fibreglass/pivc/-/issues/18, more to come

Kevin-Mattheus-Moerman commented 2 years ago

@timdewhirst, @clarka34, the author has made several updates, could you please resume the review at this point or update us on the process? Thanks a lot again for your continued help!!

quynhneo commented 2 years ago

@Kevin-Mattheus-Moerman Is there a way to document my JOSS review activity in ORCID? I found an old discussion here https://github.com/openjournals/joss/issues/813

timdewhirst commented 2 years ago

Will start taking a look again.

Kevin-Mattheus-Moerman commented 2 years ago

@quynhneo I am not sure. I've asked over at that issue you cited.

Kevin-Mattheus-Moerman commented 2 years ago

Thanks @timdewhirst, let me know if you have any questions.

Kevin-Mattheus-Moerman commented 2 years ago

@timdewhirst, @clarka34, @quynhneo. I am just checking in to see how the review is going. Could you provide an update? Thanks again for your help.

Kevin-Mattheus-Moerman commented 2 years ago

@timdewhirst, @clarka34, @quynhneo. :wave: could you give me an update please? Thanks

timdewhirst commented 2 years ago

Apologies for the delay - I'm looking again at the code. At the moment I have access to OS X and Linux environments, I can see the windows branch, but won't easily be able build on windows.

timdewhirst commented 2 years ago

I've raised another couple of minor issues, but I think my original comment stands:

"On code structure: this is my main area of concern; the code need work to follow best practices in structure, legibility, efficiency and testing. Very little C++ is used, and this is a shame; modern C++ has powerful features for generic and functional style programming which would allow the code to be cleaner, more legible, and just as efficient. Currently, I don't believe it's in good enough shape to act as a foundation for significant collaborative development."

There hasn't been much change in code structure, and as such I still believe it wouldn't be a good basis for further development. Some important areas should be addressed e.g. using gitlab's CI system, improving testing to include unit testing, improving platform independence.

On top of the concerns with the code, since the review process started I now consider myself to have a conflict of interest: my fork of OpenPIV (https://github.com/OpenPIV/openpiv-c--qt) has been merged upstream.

As such, I think it would be sensible to remove myself from the review process at this point in time.

Kevin-Mattheus-Moerman commented 2 years ago

@editorialbot remove @timdewhirst as reviewer

editorialbot commented 2 years ago

@timdewhirst removed from the reviewers list!

Kevin-Mattheus-Moerman commented 2 years ago

@timdewhirst thanks for your feedback. I have just removed you as a reviewer following the potential COI you declared. Note that I will ask the authors to address your points. Despite the fact that you are no longer able to help I do want to thank you for the help you've provided so far. Thanks.

Kevin-Mattheus-Moerman commented 2 years ago

@fibreglass2 Can you comment on @timdewhirst 's comments e.g. on code structure quality/form and on reusability/collaborative development of the code?

Kevin-Mattheus-Moerman commented 2 years ago

@clarka34, @quynhneo can you give an update on review progress please? Thanks again for your help.

quynhneo commented 2 years ago

@Kevin-Mattheus-Moerman. I will return to preview this weekend. sorry for the day.

timdewhirst commented 2 years ago

@timdewhirst thanks for your feedback. I have just removed you as a reviewer following the potential COI you declared. Note that I will ask the authors to address your points. Despite the fact that you are no longer able to help I do want to thank you for the help you've provided so far. Thanks.

It was my pleasure, and thanks for involving me in the process. Good luck!!