search

openjournals / joss-reviews

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

[REVIEW]: uDALES: large-eddy-simulation software for urban flow, dispersion, and microclimate modelling #3055

Closed whedon closed 3 years ago

whedon commented 3 years ago

Submitting author: @tomgrylls (Tom Grylls) Repository: https://github.com/uDALES/u-dales Version: 1.0.0 Editor: @Kevin-Mattheus-Moerman Reviewers: @wimvanderbauwhede, @p-costa, @ashwinvis Archive: 10.5281/zenodo.5111497

: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/b5de0d64f33555708e6893bc7e3c55d9"><img src="https://joss.theoj.org/papers/b5de0d64f33555708e6893bc7e3c55d9/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/b5de0d64f33555708e6893bc7e3c55d9/status.svg)](https://joss.theoj.org/papers/b5de0d64f33555708e6893bc7e3c55d9)

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

@wimvanderbauwhede & @p-costa & @ashwinvis, 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 @wimvanderbauwhede

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @p-costa

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @ashwinvis

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @wimvanderbauwhede, @p-costa, @ @ashwinvis 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 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.47 s (165.0 files/s, 62496.2 lines/s)
---------------------------------------------------------------------------------------
Language                             files          blank        comment           code
---------------------------------------------------------------------------------------
Fortran 90                              35           2973           3385          14709
MATLAB                                   4            572            841           3755
Markdown                                15            516              0           1305
Bourne Shell                            13            155             99            508
Python                                   3             67             61            140
CMake                                    3             40            101            125
YAML                                     3             17             13            123
TeX                                      1              2              0             31
Windows Module Definition                1              3              0              9
---------------------------------------------------------------------------------------
SUM:                                    78           4345           4500          20705
---------------------------------------------------------------------------------------

Statistical information for the repository '94b96b1e76f025d215c4983c' was
gathered on 2021/02/23.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Birgit Sutzl                     4          3509              3           11.60
Chiel                            3         11444           5722           56.69
Tom Grylls                       1            35             18            0.18
dmey                             7           287           9264           31.54

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
Tom Grylls                   17           48.6          9.8                5.88
dmey                        251           87.5         14.2               22.31
whedon commented 3 years ago

PDF failed to compile for issue #3055 with the following error:

Can't find any papers to compile :-(

Kevin-Mattheus-Moerman commented 3 years ago

@whedon generate pdf from branch joss-paper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...
whedon commented 3 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 3 years ago

@wimvanderbauwhede, @p-costa, @ashwinvis this is where the review takes place. I'll be flexible with time and will check back here in a about a month. Make sure you accept the above invitation :point_up: to be able to tick the boxes that will guide you through the review process (see also: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html).

You can comment on minor things here but we recommend that for larger issues you open an issue on the project repository under review.

Thanks again for your help! Let me know if you have any questions.

Kevin-Mattheus-Moerman commented 3 years ago

@ashwinvis our bot whedon seems to have accidentally assigned you as @ @ashwinvis initially rather than @ashwinvis (@openjournals/dev perhaps this happens sometimes when we rapidly assign one reviewer after another?). I manually fixed the above tickbox set etc to have your handle in there properly but let me know if you experience any difficulties. (I may need to reassign you).

ashwinvis commented 3 years ago

@Kevin-Mattheus-Moerman I am unable to tick the checklist.

Kevin-Mattheus-Moerman commented 3 years ago

@whedon remove @ @ashwinvis as reviewer

whedon commented 3 years ago

OK, @ @ashwinvis is no longer a reviewer

Kevin-Mattheus-Moerman commented 3 years ago

@whedon add @ashwinvis as reviewer

whedon commented 3 years ago

OK, @ashwinvis is now a reviewer

Kevin-Mattheus-Moerman commented 3 years ago

@ashwinvis can you try to accept the invite :point_up: and try again?

ashwinvis commented 3 years ago

Thanks, @Kevin-Mattheus-Moerman. It works now.

wimvanderbauwhede commented 3 years ago

@Kevin-Mattheus-Moerman Two questions:

  1. Regarding opening issues on the project repository under review: is there some tag we should use to make it clear that this is an issue related to the JOSS review?
  2. "Has the submitting author (@tomgrylls) made major contributions to the software?" In terms of LoC it is clearly "no" (0.2% of changes), all we can say is that he has several paper on the topic. What is the guidance on this?
dmey commented 3 years ago

@wimvanderbauwhede and @Kevin-Mattheus-Moerman I appreciate that the list of authors as reported by the cloc utility may lead to some confusion so let me clarify this. Most commits made during development were squashed therefore the current metadata is not very indicative of the amount of work and contributions made by each author. Furthermore given that this project is a fork of DALES, our commits appear together with those from the DALES team (we state in the summary that uDALES is a fork of DALES). @tomgrylls and @ivosuter made key contributions in terms of scientific and feature development as reflected by the current list of authors. The order of co-authors has been discussed and agreed beforehand.

Kevin-Mattheus-Moerman commented 3 years ago

@dmey thanks for the clarification.

Kevin-Mattheus-Moerman commented 3 years ago

@wimvanderbauwhede to answer your questions:

  1. No there is no specific tag to use but you could link to the issues here in a comment so we can keep track on them too. Thanks.
  2. After the above clarification I think this point is now clear and can be approved.
zbeekman commented 3 years ago

Sorry I missed the review opportunity here. If anything falls through I'd be happy to help and will try to get my email in order in the meantime (so I don't miss any notifications)... it's gotten a bit out of control over the past few months!

Kevin-Mattheus-Moerman commented 3 years ago

@zbeekman if you are keen to help I'd be happy to add you as we always welcome additional reviewers. We do already have 3 reviewers so if you are too busy then it is okay if you cannot. Let me know if you are keen then I'll add you as a reviewer.

p-costa commented 3 years ago

OK, I should mention something, @Kevin-Mattheus-Moerman -- I realize now that my former colleagues from the Aero and Hydrodynamics Laboratory at TU Delft (Netherlands), Jasper Thomas and Mathieu Pourquie (not co-authors of this paper) have contributed to this code. Nevertheless, I feel comfortable with reviewing this, and my review would surely be unbiased.

zbeekman commented 3 years ago

NOTE: Edited for clarity and wording

@Kevin-Mattheus-Moerman Thanks for the offer! In my experience having additional reviewers has never made for a quicker and more seamless review process. On the flip side, I'll informally offer any thoughts to improve the work and the authors can address them if they find it helpful.

This is my suggestion: I can work on an informal review (I'll keep a copy of the reviewer checklist markdown privately) and add comments & suggestions publically and informally to this review thread that I think could be helpful. In the event another reviewer isn't able to complete their review I can step in and become an official reviewer (and I'll be at least part of the way through the official review checklist by that point, and my comments can become official reviewer comments at that point).

Kevin-Mattheus-Moerman commented 3 years ago

OK, I should mention something, @Kevin-Mattheus-Moerman -- I realize now that my former colleagues from the Aero and Hydrodynamics Laboratory at TU Delft (Netherlands), Jasper Thomas and Mathieu Pourquie (not co-authors of this paper) have contributed to this code. Nevertheless, I feel comfortable with reviewing this, and my review would surely be unbiased.

Thanks @p-costa. That does not sound like a concern or conflict of interest to me, so that is fine. Thanks for sharing that.

Kevin-Mattheus-Moerman commented 3 years ago

@zbeekman sure that sounds fine. Thanks for your help!

whedon commented 3 years ago

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

whedon commented 3 years ago

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

wimvanderbauwhede commented 3 years ago

I opened two issues: https://github.com/uDALES/u-dales/issues/137 and https://github.com/uDALES/u-dales/issues/138

tomgrylls commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

PDF failed to compile for issue #3055 with the following error:

Can't find any papers to compile :-(

tomgrylls commented 3 years ago

@whedon generate pdf from branch joss-paper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...
whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

tomgrylls commented 3 years ago

@whedon generate pdf from branch joss-paper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...
whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

p-costa commented 3 years ago

Hi @tomgrylls ,

I just read the article and it looks great. I have no major comments. Just two minor things based on the last proof (https://github.com/openjournals/joss-reviews/issues/3055#issuecomment-803294401):

  1. Line 35 -- F in Fast should not be capitalized and the dash can be removed.
  2. Line 128 -- Bibliographic details of the PhD thesis are missing?

Now, I have a question regarding the acknowledgement of other people who may have contributed to uDALES (i.e. the fork of DALES), who are not co-authors. Perhaps I'm wrong but I think that Jasper Tomas, Mathieu Pourquie, and maybe Harmen Jonker, have contributed to it by adding e.g. an immersed boundary and changing the Poisson solver?

These contributions are acknowledged in the source code, but I feel they should be made more explicit because the average user will never see them (they are also not in the commit history). In short, I wonder if there are other important contributors to uDALES who should be mentioned e.g. in a list of contributors on the root of the repo, acknowledged in the paper, etc, so that their work gets credited?

EDIT: In addition, I feel it would be good to mention in the paper/docs the type of geometries that can be accommodated as an immersed-boundary using the stress IBM, since this is a key feature of the code. The geometry needs to conform to the underlying grid, so I think that only cuboids or tetrahedrons are supported?

ashwinvis commented 3 years ago

Hi, Some questions and comments for @tomgrylls, before I check off my review list.

Notes

Corresponding to the line numbers from the latest manuscript

The main modifications are the addition of an immersed boundary method (Pourquie et al. (2009)), the implementation of inflow/outflow boundary conditions and the application of the eddy-viscosity subgrid model of Vreman (2004).

Here I have the same concern as @p-costa. It appears that the authors of the 2015 article contributed to the development of uDALES. If so, why are they not acknowledged here?

On documentation    

dmey commented 3 years ago

@ashwinvis thanks for your feedback -- I can address your docs comments but will leave the others for @tomgrylls.

Installation instruction can be improved

Can you be a bit more specific? What exactly needs to be improved? We have recently address the @wimvanderbauwhede's docs related comments in https://github.com/uDALES/u-dales/issues/138.

Tests instructions (now in a standalone README.md) should be included in the main documentation

Tests instructions are linked from the development guide which is itself linked from the main README.md. Software testing is less of a end-user facing task. This was reported by @wimvanderbauwhede in https://github.com/uDALES/u-dales/issues/137. I don't see a good reason for adding it to the main README (will make it unnecessarily bloated) but please let me know if you/others think otherwise.

Contributing guidelines can be included in the main documentation too.

Same as above -- we kept it separate and linked from the main README to keep it lean.

ashwinvis commented 3 years ago

I will follow up on installation issues in uDALES/u-dales#138.

Software testing is less of a end-user facing task.

Since these are research software very often the line between a user and a developer is very fuzzy. So it is a matter of taste if you want to have good on-boarding or not.

which is itself linked from the main README.md

As for testing and contributing notes, it is definitely linked in the README, but the README is not included in the main documentation https://udales.github.io/u-dales/. Right now if I search for either in the documentation search bar, I get nothing, which may lead to wrong impression for the reader. For now, I think a link to tests and contributing guidelines would be appreciated.

dmey commented 3 years ago

Since these are research software very often the line between a user and a developer is very fuzzy. So it is a matter of taste if you want to have good on-boarding or not.

which is itself linked from the main README.md

As for testing and contributing notes, it is definitely linked in the README, but the README is not included in the main documentation https://udales.github.io/u-dales/. Right now if I search for either in the documentation search bar, I get nothing, which may lead to wrong impression for the reader. For now, I think a link to tests and contributing guidelines would be appreciated.

I see what you mean -- I am addressing this at https://github.com/uDALES/u-dales/pull/141

tomgrylls commented 3 years ago

@whedon generate pdf from branch joss-paper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...
whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

tomgrylls commented 3 years ago

@p-costa @ashwinvis Thanks so much for your feedback and comments. I am just finalising a couple of updates and responses with the the other authors and I will provide a detailed response to all of the above soon.

tomgrylls commented 3 years ago

Thanks @p-costa for your questions, apologies for the delay in getting back to you on this. Please see our response below.

  1. Line 35 -- F in Fast should not be capitalized and the dash can be removed.

Thanks for pointing out - now amended

  1. Line 128 -- Bibliographic details of the PhD thesis are missing?

These have now been updated. A couple of the PhD theses are still under embargo following completion. I have indicated where this is the case in the bibliography.

Now, I have a question regarding the acknowledgement of other people who may have contributed to uDALES (i.e. the fork of DALES), who are not co-authors. Perhaps I'm wrong but I think that Jasper Tomas, Mathieu Pourquie, and maybe Harmen Jonker, have contributed to it by adding e.g. an immersed boundary and changing the Poisson solver?

These contributions are acknowledged in the source code, but I feel they should be made more explicit because the average user will never see them (they are also not in the commit history). In short, I wonder if there are other important contributors to uDALES who should be mentioned e.g. in a list of contributors on the root of the repo, acknowledged in the paper, etc, so that their work gets credited?

Thanks for pointing this out. We have updated both the paper and README files to better acknowledge their contributions. Specifically, changed the text in the paper:

'The immersed boundary method, first introduced into DALES by @Pourquie2009 and @Tomas2015, is used to represent buildings (supporting grid-conforming cuboid geometries; @Pourquie2009).'

Update to the README, including a new acknowledgements section:

'The immersed boundary method, which is used to model obstacles in the fluid domain, was implemented into DALES and validated by Pourquie et al. (2009) and Tomas et al. (2015).'

'## Acknowledgements

The dynamic core of uDALES derives from the Dutch Atmospheric Large-Eddy Simulation (DALES) model. We would like to acknowledge the work of the contributors to DALES over the years (Heus et al. 2010) and specifically the work of J. Tomas, M. Pourquie and H. Jonker in implementing the immersed boundary method into DALES (Tomas et al. 2015).'

EDIT: In addition, I feel it would be good to mention in the paper/docs the type of geometries that can be accommodated as an immersed-boundary using the stress IBM, since this is a key feature of the code. The geometry needs to conform to the underlying grid, so I think that only cuboids or tetrahedrons are supported?

Thanks for pointing this out. I have updated the manuscript to make this clearer. I have also added in some more information in the uDALES set-up documentation and a reference to this on the documentation for example 201 where it advises on pre-defined buildings.

As you mention, the implementation of the IBM is such that the buildings must conform to the underlying grid and therefore only cuboids are currently supported in uDALES. The advantage of this implementation is that it is second-order accurate, the grid is relatively simple to generate and the pressure field can be solved via a fast Fourier-based transform method (the latter provides a big performance advantage). We have validated the use of grid conforming buildings for both idealised and realistic urban morphologies with good results. We are planning to update the IBM implementation in the future to capture more complex forms. As mentioned, the grid-conforming limitation has now been made clearer in the paper and documentation.

tomgrylls commented 3 years ago

Thanks @ashwinvis for your questions, apologies for the delay in getting back to you on this. Please see our response below.

  • 19: Substantiate the claim of typical spatial and temporal scales involved in an urban LES. Smallest scale of O(1 m) seems quite large to me. Also, O(1 h) seems too less as it will not capture a single diurnal cycle. See Figure 3 https://doi.org/10.5194/gmd-13-1335-2020 for instance.

The resolution of O(1 m) is representative of the order of magnitude of resolution used across the current publications that have used uDALES to date. The range used across these is ~0.25-2 m. The use of wall functions means that refinement of the grid close to solid-fluid boundaries, as done in other urban LES, is not necessary. In terms of capturing typical urban morphologies, studies have shown that a minimum of 16 cells are required per building to capture flows in the urban canopy layer, with a resolution of 1 m this corresponds to building heights of approximately 16 m or larger, which is reasonable.

Agreed that that the choice of O(1 h) here is a misrepresentation of the general application of the code. For urban studies typically 10s of hours are needed to obtain statistically steady results. I have updated this to be 'O(10 h)', which is consistent with many of the simulations in the papers published using uDALES to date. Simulations of both O(1 h) and O(100 h) are also feasible using uDALES.

  • 24: Explicitly mention the key feature which differentiates uDALES and DALES. I had to find it from Sützl et al. (2020) which says, uDALES was extended to provide "the capability to model buildings using an immersed boundary method". Also a "statement of need" section is missing from the manuscript.

I have updated the manuscript to explicitly make this point at the start of the second paragraph (see lines 25-27). The remainder of the manuscript mentions the other features that have been added to the code in more detail.

The requirement for a statement of need was discussed with the editor and reviewer twice previously here, where they agreed that an additional section was not necessary as the statement of need was clear from the main body of text in the manuscript. Are you also okay with the statement of need being made within the text (see lines 9-23)?

  • 30: Reference for kappa-scheme is missing.

Thanks for highlighting - reference added.

  • 36: "fully parallelized". Mention what kind of domain decomposition is used. Slabs or pencils? Which directions are distributed over MPI ranks?

Updated the document to make this clear. Domain decomposition is performed in the spanwise direction, thereby producing slabs.

  • 44: "study real-time pollution dispersion". It is unclear to me what real-time means here.

Real-time in this sense referred to the fact that uDALES time-resolves the turbulent flow field and therefore the dispersion of pollutants unlike Reynolds-averaged Navier-Stokes (RANS) models that provide time averaged concentrations. It is therefore possible to study e.g. pedestrian exposure to pollution in 'real time'.

Agreed that this terminology was not clear. 'real time pollution dispersion' has been replaced by 'air quality' more generally (this is then inclusive of pollution emissions, dispersion, reaction and deposition). A sentence has been added to highlight the ability of the model to capture 'real time pedestrian exposure'.

  • 74: URL missing for Grylls (2020)

This thesis was placed under an embargo for 18 months following completion. The thesis will be made available in July 2021. I have added additional detail to the reference list and put a note of "(under embargo)" to capture this point.

  • 130: URL / DOI missing for Suter et al. (2021). Is it submitted and pending review? I am curious about this article as it is closely related to this manuscript. Do you have a preprint available?

This paper is still in preparation - I have added a note of (in preparation) to the reference list. The paper outlines the scientific details of the developments made in uDALES in more detail and also presents further validations of the code. Our hope is that that paper alongside this one will provide a complete overview of the uDALES software.

We do not yet have a preprint available to share. It is likely we will have a preprint available within the next 3 months once the paper has been finalised and submitted.

  • 136: Tomas et al. (2015) mentions

The main modifications are the addition of an immersed boundary method (Pourquie et al. (2009)), the implementation of inflow/outflow boundary conditions and the application of the eddy-viscosity subgrid model of Vreman (2004).

Here I have the same concern as @p-costa. It appears that the authors of the 2015 article contributed to the development of uDALES. If so, why are they not acknowledged here?

Replied in detail to the same question here

tomgrylls commented 3 years ago

@whedon generate pdf from branch joss-paper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...
whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left: