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

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

Closed
whedon opened this issue Aug 11, 2021 · 54 comments
Assignees
Labels
C++ C Makefile pre-review Track: 7 (CSISM) Computer science, Information Science, and Mathematics

Comments

@whedon
Copy link

whedon commented Aug 11, 2021

Submitting author: @fibreglass2 (Kadeem)
Repository: https://gitlab.com/fibreglass/pivc
Version: v1.0.0
Editor: @Kevin-Mattheus-Moerman
Reviewers: @timdewhirst, @clarka34, @quynhneo
Managing EiC: Arfon Smith

⚠️ JOSS reduced service mode ⚠️

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)

Author instructions

Thanks for submitting your paper to JOSS @https://gitlab.com/fibreglass. Currently, there isn't an JOSS editor assigned to your paper.

@https://gitlab.com/fibreglass if you have any suggestions for potential reviewers then please mention them here in this thread (without tagging them with an @). In addition, this list of people have already agreed to review for JOSS and may be suitable for this submission (please start at the bottom of the list).

Editor instructions

The JOSS submission bot @whedon is here to help you find and assign reviewers and start the main review. To find out what @whedon can do for you type:

@whedon commands
@whedon
Copy link
Author

whedon commented Aug 11, 2021

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks.

⚠️ JOSS reduced service mode ⚠️

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.

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
Copy link
Author

whedon commented Aug 11, 2021

Wordcount for paper.md is 946

@whedon
Copy link
Author

whedon commented Aug 11, 2021

Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.03 s (514.8 files/s, 60230.3 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C++                              4             72             78            794
TeX                              1             33              0            245
Markdown                         3             47              0            169
C/C++ Header                     4             15              1             59
make                             1              1              0              7
-------------------------------------------------------------------------------
SUM:                            13            168             79           1274
-------------------------------------------------------------------------------


Statistical information for the repository 'f1d7693b5c2b55677911fe74' was
gathered on 2021/08/11.
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
Copy link
Author

whedon commented Aug 11, 2021

👉📄 Download article proof 📄 View article proof on GitHub 📄 👈

@whedon
Copy link
Author

whedon commented Aug 11, 2021

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.5334/jors.334 is OK
- 10.5334/jors.bl is OK

MISSING DOIs

- 10.1007/s003489900085 may be a valid DOI for title: Comparison of Gaussian particle center estimators and the achievable measurement density for particle tracking velocimetry
- 10.1088/0169-5983/47/3/035509 may be a valid DOI for title: The characteristics of coherent structures in low Reynolds number mixed convection flows
- 10.1016/j.ijthermalsci.2013.11.001 may be a valid DOI for title: The influence of bottom wall heating on the mean and turbulent flow behavior in the near wall region during mixed convection
- 10.1063/1.1375144 may be a valid DOI for title: Simultaneous particle image velocimetry and infrared imagery of microscale breaking waves
- 10.1016/j.expthermflusci.2020.110286 may be a valid DOI for title: An experimental study of the aerodynamics of micro corrugated wings at low Reynolds number
- 10.1016/j.est.2019.100825 may be a valid DOI for title: Investigation of the influence of heat source orientation on the transient flow behavior during PCM melting using particle image velocimetry
- 10.1016/j.ijheatfluidflow.2021.108839 may be a valid DOI for title: The influence of wall heating on turbulent boundary layer characteristics during mixed convection
- 10.1016/j.ijheatfluidflow.2008.01.003 may be a valid DOI for title: The influence of wall heating on the flow structure in the near-wall region
- 10.1063/1.2185709 may be a valid DOI for title: Turbulent Structure beneath Air-Water Interface during Natural Convection
- 10.1007/s00231-005-0072-8 may be a valid DOI for title: Characteristics of Air and Water Velocity Fields during Natural Convection
- 10.1063/1.3054153 may be a valid DOI for title: An Experimental Study of the Airside Flow Structure during Natural Convection
- 10.1088/0957-0233/18/1/012 may be a valid DOI for title: Velocity measurements around a freely swimming fish using PIV
- 10.1017/s0022112006003892 may be a valid DOI for title: Characteristics of the Wind Drift Layer and Microscale Breaking Waves
- 10.1016/j.ijthermalsci.2015.05.003 may be a valid DOI for title: Investigation of fundamental flow mechanisms over a corrugated waveform using proper orthogonal decomposition and spectral analyses
- 10.1016/j.jweia.2021.104605 may be a valid DOI for title: Turbulent flow characterization near the edge of a steep escarpment
- 10.1002/we.1895 may be a valid DOI for title: Flow Characterization in the near-wake region of a horizontal axis wind turbine
- 10.1007/s10236-008-0132-y may be a valid DOI for title: Airside velocity measurements over the wind-sheared water surface using Particle Image Velocimetry

INVALID DOIs

- None

@arfon arfon added the paused label Aug 11, 2021
@arfon
Copy link
Member

arfon commented Aug 11, 2021

Pausing as we don't have a GitHub handle for the author (I've emailed them about this).

Also, querying scope due to size.

@arfon
Copy link
Member

arfon commented Aug 11, 2021

@whedon query scope

@whedon whedon added the query-scope Submissions of uncertain scope for JOSS label Aug 11, 2021
@whedon
Copy link
Author

whedon commented Aug 11, 2021

Submission flagged for editorial review.

@fibreglass2
Copy link

fibreglass2 commented Aug 11, 2021

Pausing as we don't have a GitHub handle for the author (I've emailed them about this).

This is my Github account.

Also, querying scope due to size.

Take caution when observing the current line of code count. This program was originally much larger in terms of LoC, total files, and written to utilize a proprietary closed-source API. My coauthors and I started this work in 2017 to optimize the original code as much as possible, hence greatly reducing the total files and LoC present, obtain gains in computation speed, and eliminate the requirement of a proprietary closed-source API.

@Kevin-Mattheus-Moerman
Copy link
Member

Kevin-Mattheus-Moerman commented Aug 30, 2021

@fibreglass2 thanks for this submission. I quickly scanned through your paper as part of the scope review. Some quick comments:

  • Typo: commerical
  • Can you work on fixing those missing DOI's ☝️
  • Can you comment, here and/or in paper, if the current MATLAB libraries are Octave compatible? If so you can call them inefficient perhaps but they could be compatible with an open source run-time. If you are unsure or cannot find this information you can leave the language in the paper as is.
  • If you feel your library offers a significant speed up over existing libraries perhaps you could consider adding a performance comparison for a particular example data set (not a requirement just something to really sell you library).

If you make any changes to the paper or bib file you can call the following in a comment to update the paper:
@whedon generate pdf
and to to check references:
@whedon check references

@kyleniemeyer
Copy link

@fibreglass2 we're going to go ahead and proceed with review on this one—can you please address the comments that @Kevin-Mattheus-Moerman raised?

@kyleniemeyer kyleniemeyer removed the query-scope Submissions of uncertain scope for JOSS label Sep 3, 2021
@Kevin-Mattheus-Moerman
Copy link
Member

@kyleniemeyer FYI I can edit this one if you like

@kyleniemeyer
Copy link

@whedon assign @Kevin-Mattheus-Moerman as editor

@Kevin-Mattheus-Moerman sure thing! Wasn't going to ask but happy to accept the offer.

@whedon
Copy link
Author

whedon commented Sep 4, 2021

OK, the editor is @Kevin-Mattheus-Moerman

@fibreglass2
Copy link

@fibreglass2 we're going to go ahead and proceed with review on this one—can you please address the comments that @Kevin-Mattheus-Moerman raised?

Certainly. Expect to see updates soon.

@fibreglass2
Copy link

@whedon generate pdf

@whedon
Copy link
Author

whedon commented Sep 6, 2021

👉📄 Download article proof 📄 View article proof on GitHub 📄 👈

@fibreglass2
Copy link

@whedon check references

@whedon
Copy link
Author

whedon commented Sep 6, 2021

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1007/s003489900085 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.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

MISSING DOIs

- 10.1007/s00348-016-2173-1 may be a valid DOI for title: Main results of the 4th International PIV Challenge
- 10.1016/j.ijheatfluidflow.2013.12.005 may be a valid DOI for title: The effect of mixed convection on the structure of channel flow at low Reynolds numbers
- 10.1016/j.ijthermalsci.2014.11.026 may be a valid DOI for title: Flow development during low Reynolds number mixed convection

INVALID DOIs

- None

@fibreglass2
Copy link

@whedon check references

@whedon
Copy link
Author

whedon commented Sep 6, 2021

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

@fibreglass2
Copy link

@whedon generate pdf

@clarka34
Copy link

@Kevin-Mattheus-Moerman I worked on the installation today and was able to use both of the precompiled versions (windows + linux) and then compiled it successfully in windows (although with warnings). I did not have any luck compiling in linux, but I think that was a problem with the dependencies. I found a bunch of minor things that could be improved in the documentation. Do I raise issues individually or group them?

@Kevin-Mattheus-Moerman
Copy link
Member

@clarka34 thanks for doing that. If you would be okay with it it would be great if I can assign you as a reviewer, it sounds like you'd be able to help.

This issue here is a pre-review issue. Once I have found enough reviewers I will open a dedicated review issue (which will show tickboxes to guide your review). Also we encourage reviewers to open issues on the software repository. You can group things or separate as you see fit. Then please refer to these opened issues in the review issue.

Would it be okay to assign you as a reviewer at this point?

@Kevin-Mattheus-Moerman
Copy link
Member

Kevin-Mattheus-Moerman commented Sep 17, 2021

@yosefm @eguvep @dcsale would you be interested in reviewing this work for JOSS?

The review focuses on the software itself, as well as a short paper. The streamlined review process takes place on GitHub (here is an example of an ongoing review: #3611 (comment)).

Let me know if you are interested. Note we can be flexible regarding time taken to review.

@clarka34
Copy link

@Kevin-Mattheus-Moerman Sure thing! Just to be clear though, I definitely cannot review the code itself

@Kevin-Mattheus-Moerman
Copy link
Member

@whedon add @clarka34 as reviewer

@whedon
Copy link
Author

whedon commented Sep 18, 2021

OK, @clarka34 is now a reviewer

@Kevin-Mattheus-Moerman
Copy link
Member

@Kevin-Mattheus-Moerman Sure thing! Just to be clear though, I definitely cannot review the code itself

@clarka34 thanks, that is fine. We will also include other reviewers that will look into the code structure itself. However, a line-by-line code review is not really expected. If you focus on install, documentation, and evaluation of the functionality, that would be great.
I will hopefully recruit one more reviewer before I'll trigger the formal review process. Feel free to start already if you like and post issues on this submission's software repository, please post links to any issues later in the review issue. Thanks again!

@Kevin-Mattheus-Moerman
Copy link
Member

@quynhneo @ZIQIANG059 @vitorvilela @lento234 @yosefm @eguvep @dcsale would you be interested in reviewing this work for JOSS?

The review focuses on the software itself, as well as a short paper. The streamlined review process takes place on GitHub (here is an example of an ongoing review: #3611 (comment)).

Let me know if you are interested. Note we can be flexible regarding time taken to review.

@quynhneo
Copy link

@Kevin-Mattheus-Moerman Yeah I would. Which aspect of the code would you like my review?

@timdewhirst
Copy link

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
Copy link
Member

@whedon add @quynhneo as reviewer

@whedon
Copy link
Author

whedon commented Sep 18, 2021

OK, @quynhneo is now a reviewer

@Kevin-Mattheus-Moerman
Copy link
Member

@whedon start review

@whedon
Copy link
Author

whedon commented Sep 18, 2021

OK, I've started the review over in #3736.

@dcsale
Copy link

dcsale commented Sep 26, 2021 via email

@editorialbot editorialbot added the Track: 7 (CSISM) Computer science, Information Science, and Mathematics label Sep 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ C Makefile pre-review Track: 7 (CSISM) Computer science, Information Science, and Mathematics
Projects
None yet
Development

No branches or pull requests