openjournals / joss-reviews

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

[REVIEW]: Triumvirate: A Python/C++ package for three-point clustering measurements #5571

Closed editorialbot closed 11 months ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@MikeSWang<!--end-author-handle-- (Mike Shengbo Wang) Repository: https://github.com/MikeSWang/Triumvirate Branch with paper.md (empty if default branch): Version: v0.3.0 Editor: !--editor-->@ivastar<!--end-editor-- Reviewers: @alfonso-veropalumbo, @wcoulton Archive: 10.5281/zenodo.10072128

Status

status

Status badge code:

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

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

@alfonso-veropalumbo & @wcoulton, 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:

@editorialbot generate my checklist

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

📝 Checklist for @alfonso-veropalumbo

editorialbot commented 1 year ago

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:

@editorialbot commands

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

@editorialbot generate pdf
editorialbot commented 1 year ago
Software report:

github.com/AlDanial/cloc v 1.88  T=0.16 s (752.3 files/s, 230036.6 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C++                             12           1440           1092           6711
Python                          25           1756           3137           4879
CSS                              4            453            161           2033
Cython                           9            366            468           1125
YAML                            14            206            212            803
C/C++ Header                    11            372           2243            757
Markdown                        11            241              0            524
Jupyter Notebook                 7              0           3961            451
reStructuredText                 8            277            272            305
TeX                              2             22              0            247
make                             2            129             91            217
JavaScript                       4             56            103            215
Bourne Shell                     4             64            112            169
INI                              2             56              0            148
TOML                             1             13              2             92
HTML                             2              3             32             68
-------------------------------------------------------------------------------
SUM:                           118           5454          11886          18744
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 1 year ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1093/mnras/sty3249 is OK
- 10.1093/mnras/stx2333 is OK
- 10.1103/PhysRevD.95.063508 is OK
- 10.48550/arXiv.1611.00036 is OK
- 10.48550/arXiv.1110.3193 is OK
- 10.1007/s41114-017-0010-3 is OK
- 10.1016/s0370-1573(02)00135-7 is OK
- 10.1103/PhysRevD.74.023522 is OK
- 10.1093/mnras/stx721 is OK
- 10.1103/PhysRevD.103.083533 is OK
- 10.1086/307220 is OK
- 10.1103/PhysRevD.92.083532 is OK
- 10.1093/mnras/sty1063 is OK
- 10.1093/pasj/58.1.93 is OK
- 10.1093/mnras/stw2576 is OK
- 10.1201/9780367806934 is OK
- 10.3847/1538-3881/aadae0 is OK
- 10.1093/mnras/stw1229 is OK
- 10.1086/427087 is OK
- 10.3847/1538-4357/ac7c74 is OK
- 10.1086/174036 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 1 year ago

Wordcount for paper.md is 1615

editorialbot commented 1 year ago

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

wcoulton commented 1 year ago

Review checklist for @wcoulton

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

wcoulton commented 1 year ago

Dear @MikeSWang,

In general I think that this package is amazing! It has lots of useful functionality and it is easy to install and use. I can certainly see myself using this in future projects! 

I had four minor comments:

  1. I think it would be useful to add a little more description to the returns of the functions in the documentation. A few function outputs are just described as "results – Measurement results". The actual returned dictionary has a few parts and it could be useful to either describe that there or link to the examples where the output is explained in detail. 
  2. Unless I have misunderstood, it seems necessary to repeatedly run the program, iteratively updating idx_bin, to compute all the elements in the full 2D bispectrum. For my applications, and I suspect that of some other users, the full bispectrum would often be of interest. Perhaps a function could be added that returns this? 
  3. Perhaps in the background material you could add a sentence or two more summarizing the window functions?
  4. In your supporting paper, I think it may be useful to add a reference to, and discussion of, some other codes that have some similar functionality. The first one that jumps first to mind is: https://github.com/oliverphilcox/Spectra-Without-Windows

Super minor: there seems to be an example folder in the repo but it is empty?

This package is wonderful, congratulations! My apologies for the slow review.

alfonso-veropalumbo commented 1 year ago

Review checklist for @alfonso-veropalumbo

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

MikeSWang commented 1 year ago

Thanks @wcoulton for your valuable feedback!

I just came back from a conference and will be travelling again soon, but I will incorporate your suggestions especially with regard to number 2 (you're right about it; I have just opened a new feature request https://github.com/MikeSWang/Triumvirate/issues/22).

A quick response to the last comment: the examples folder is a placeholder, and I meant to collect users' scripts for different use cases like a 'gallery', which may not be formal enough to be included in the official documentation :)

Dear @MikeSWang,

In general I think that this package is amazing! It has lots of useful functionality and it is easy to install and use. I can certainly see myself using this in future projects! 

I had four minor comments:

  1. I think it would be useful to add a little more description to the returns of the functions in the documentation. A few function outputs are just described as "results – Measurement results". The actual returned dictionary has a few parts and it could be useful to either describe that there or link to the examples where the output is explained in detail.
  2. Unless I have misunderstood, it seems necessary to repeatedly run the program, iteratively updating idx_bin, to compute all the elements in the full 2D bispectrum. For my applications, and I suspect that of some other users, the full bispectrum would often be of interest. Perhaps a function could be added that returns this?
  3. Perhaps in the background material you could add a sentence or two more summarizing the window functions?
  4. In your supporting paper, I think it may be useful to add a reference to, and discussion of, some other codes that have some similar functionality. The first one that jumps first to mind is: https://github.com/oliverphilcox/Spectra-Without-Windows

Super minor: there seems to be an example folder in the repo but it is empty?

This package is wonderful, congratulations! My apologies for the slow review.

ivastar commented 1 year ago

@alfonso-veropalumbo thank you for going through the checklist! If you have any comments in addition to those previously listed, please add them here. If not, let me and the authors know that they can move to responding to the review.

alfonso-veropalumbo commented 1 year ago

Dear @MikeSWang

First of all, thank you for your patience. I'm sorry for this delayed answer.

I've had the chance to test Triumvirate, and I've particularly appreciated the ease of use, the completeness of the documentation and the high performance in dealing with this task. Moreover, the advantage of Triumvirate is that it allows computation of the anisotropic three-point correlation function (3PCF), fully exploiting the information encoded in the higher orders moment of the galaxy density field. This aspect constitutes a new feature (apart from Triumvirate predecessor, Hitomi) and a critical advancement of the clustering field.

I have minor suggestions related to the paper:

1) quantify performances in the paper. 3PCF computation is generally thought to be (and for large samples, it is) an expensive procedure, and it could be helpful to report the performances of this specific task. 2) improve the comparison with other codes on the market (e.g. https://arxiv.org/abs/2105.08722). On this point, I have a doubt about the description of nbodykit made in the draft. Looking at the documentation (https://nbodykit.readthedocs.io/en/latest/api/_autosummary/nbodykit.algorithms.threeptcf.html#nbodykit.algorithms.threeptcf.SimulationBox3PCF) nbodykit implements the Slepian 2015 procedure, which is not assigning spherical harmonics to the mesh grid. This procedure computes the spherical harmonics on a pair-to-pair basis. As a result, it should be more accurate but significantly slower.

Apart from these minor comments, thank you again for this work, which is important for the clustering community. I hope packages like this will help people approach this field, which will gain more and more importance for clustering analyses in cosmology.

Sorry again for the delayed comments. I'm available in case further clarifications are needed.

ivastar commented 1 year ago

Thank you @alfonso-veropalumbo for the detailed review! @MikeSWang the ball is in your court. Please let us all know when you've had a chance to address the comments.

MikeSWang commented 1 year ago

Thank you all for the updates! I'm back from travelling and will address these comments in the coming weeks.

A quick question about comment 1 from @alfonso-veropalumbo: we have included some performance metrics for the bispectrum, so I guess you are asking about the 3PCF in configuration space in particular?

And a quick reply to comment 2: nbodykit's 3PCF algorithm does use the actual lines of sight, and in the paper we were thinking about its 2-point algorithms... Given it would be better to compare with its 3PCF, I will clarify this in the text.

arfon commented 1 year ago

Friendly reminder to get to these updates soon please @MikeSWang. @alfonso-veropalumbo – I think the author asked you a question here too...

A quick question about comment 1 from @alfonso-veropalumbo: we have included some performance metrics for the bispectrum, so I guess you are asking about the 3PCF in configuration space in particular?

MikeSWang commented 1 year ago

Hi @arfon thanks for the reminder. Indeed as I am writing now I am working on resubmitting the new draft and making a new release, aiming for the coming week. Apologies summer travel and other things have delayed my response. I have also added other optimisations to the code not asked for by the reviewer, hence the new version release has taken longer.

MikeSWang commented 1 year ago

I actually have a question about updating the draft @arfon @ivastar. It will be available in a new commit in my own repo, but how do I get the JOSS editorial bot to compile it here?

arfon commented 1 year ago

@editorialbot generate pdf

Anyone here can do this ☝️ @MikeSWang

editorialbot commented 1 year ago

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

alfonso-veropalumbo commented 1 year ago

Friendly reminder to get to these updates soon please @MikeSWang. @alfonso-veropalumbo – I think the author asked you a question here too...

A quick question about comment 1 from @alfonso-veropalumbo: we have included some performance metrics for the bispectrum, so I guess you are asking about the 3PCF in configuration space in particular?

Yes, sure, I think it could be useful. Computational time is the main issue, computationally speaking, for configuration space estimators (and particularly for 3PCF).

MikeSWang commented 1 year ago

@editorialbot commands

editorialbot commented 1 year ago

Hello @MikeSWang, here are the things you can ask me to do:


# List all available commands
@editorialbot commands

# Get a list of all editors's GitHub handles
@editorialbot list editors

# Check the references of the paper for missing DOIs
@editorialbot check references

# Perform checks on the repository
@editorialbot check repository

# Adds a checklist for the reviewer using this command
@editorialbot generate my checklist

# Set a value for branch
@editorialbot set joss-paper as branch

# Generates the pdf paper
@editorialbot generate pdf

# Generates a LaTeX preprint file
@editorialbot generate preprint

# Get a link to the complete list of reviewers
@editorialbot list reviewers
MikeSWang commented 1 year ago

@editorialbot check references @editorialbot check repository

editorialbot commented 1 year ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1093/mnras/sty3249 is OK
- 10.1093/mnras/stx2333 is OK
- 10.1103/PhysRevD.95.063508 is OK
- 10.48550/arXiv.1611.00036 is OK
- 10.48550/arXiv.1110.3193 is OK
- 10.1007/s41114-017-0010-3 is OK
- 10.1016/s0370-1573(02)00135-7 is OK
- 10.1103/PhysRevD.74.023522 is OK
- 10.1093/mnras/stx721 is OK
- 10.1103/PhysRevD.103.083533 is OK
- 10.1086/307220 is OK
- 10.1093/pasj/58.1.93 is OK
- 10.1093/mnras/stw2576 is OK
- 10.1103/PhysRevD.92.083532 is OK
- 10.1093/mnras/stv2119 is OK
- 10.1093/mnras/sty1063 is OK
- 10.1093/mnrasl/slv133 is OK
- 10.1103/physrevd.104.123529 is OK
- 10.1201/9780367806934 is OK
- 10.3847/1538-3881/aadae0 is OK
- 10.1093/mnras/stw1229 is OK
- 10.1086/427087 is OK
- 10.3847/1538-4357/ac7c74 is OK
- 10.1086/174036 is OK

MISSING DOIs

- None

INVALID DOIs

- None
MikeSWang commented 1 year ago

@editorialbot check references

editorialbot commented 1 year ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1093/mnras/sty3249 is OK
- 10.1093/mnras/stx2333 is OK
- 10.1103/PhysRevD.95.063508 is OK
- 10.48550/arXiv.1611.00036 is OK
- 10.48550/arXiv.1110.3193 is OK
- 10.1007/s41114-017-0010-3 is OK
- 10.1016/s0370-1573(02)00135-7 is OK
- 10.1103/PhysRevD.74.023522 is OK
- 10.1093/mnras/stx721 is OK
- 10.1103/PhysRevD.103.083533 is OK
- 10.1086/307220 is OK
- 10.1093/pasj/58.1.93 is OK
- 10.1093/mnras/stw2576 is OK
- 10.1103/PhysRevD.92.083532 is OK
- 10.1093/mnras/stv2119 is OK
- 10.1093/mnras/sty1063 is OK
- 10.1093/mnrasl/slv133 is OK
- 10.1103/physrevd.104.123529 is OK
- 10.1201/9780367806934 is OK
- 10.3847/1538-3881/aadae0 is OK
- 10.1093/mnras/stw1229 is OK
- 10.1086/427087 is OK
- 10.3847/1538-4357/ac7c74 is OK
- 10.1086/174036 is OK

MISSING DOIs

- None

INVALID DOIs

- None
MikeSWang commented 1 year ago

@editorialbot check repository

editorialbot commented 1 year ago
Software report:

github.com/AlDanial/cloc v 1.88  T=0.23 s (539.2 files/s, 171570.0 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C++                             12           1658           1115           8095
Python                          27           1798           3238           5115
CSS                              4            453            161           2033
Cython                          11            391            517           1201
YAML                            14            216            304            817
C/C++ Header                    11            388           2304            799
Markdown                        11            291              0            637
reStructuredText                 9            355            290            441
Jupyter Notebook                 7              0           4088            439
TeX                              2             26              0            284
make                             2            137            100            256
JavaScript                       4             56            103            215
INI                              2             60              0            201
Bourne Shell                     4             64            112            169
TOML                             1             13              2             92
HTML                             2              3             32             68
-------------------------------------------------------------------------------
SUM:                           123           5909          12366          20862
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 1 year ago

Wordcount for paper.md is 2080

MikeSWang commented 1 year ago

Dear editor @ivastar and reviewers @wcoulton & @alfonso-veropalumbo,

Many thanks for your feedback and patience. I am happy to announce that

are now both available.

To address your questions in turn:

In the meantime, there have been other bug fixes, additional features, performance enhancement, code maintenance and documentation improvements since the reviewed release.

Let me know if you have any questions and many thanks again!

On behalf of all authors, @MikeSWang

MikeSWang commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

ivastar commented 1 year ago

@MikeSWang thank you for making the changes recommended by the reviewer!

@wcoulton & @alfonso-veropalumbo, please review the changes and if you are satisfied with the code and manuscript, please comment that you accept the submission for publication.

Otherwise, comment further on the relevant issues or in this thread to recommend further changes.

ivastar commented 1 year ago

@wcoulton & @alfonso-veropalumbo, pinging this thread again. I would appreciate it if you could sign off on the changes so we can move to publication.

wcoulton commented 11 months ago

This all looks good to me and I am happy to recommend that the submission for publication. Great work!!

alfonso-veropalumbo commented 11 months ago

Everything looks good to me! Green light from my side for publication. Thank you very much for your work!

MikeSWang commented 11 months ago

Thank you @wcoulton, @alfonso-veropalumbo and @ivastar for your work and feedback too!

ivastar commented 11 months ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Editor Tasks Prior to Acceptance

ivastar commented 11 months ago

@editorialbot generate pdf

editorialbot commented 11 months ago

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

MikeSWang commented 11 months ago

I can confirm the following:

Additional Author Tasks After Review is Complete

  • [x] Double check authors and affiliations (including ORCIDs)
  • [x] Make a release of the software with the latest changes from the review and post the version number here. This is the version that will be used in the JOSS paper. ---> Version 0.3.0
  • [x] Archive the release on Zenodo/figshare/etc and post the DOI here. ---> 10.5281/zenodo.10072128
  • [x] Make sure that the title and author list (including ORCIDs) in the archive match those in the JOSS paper.
  • [x] Make sure that the license listed for the archive is the same as the software license.
ivastar commented 11 months ago

Thanks @MikeSWang! I am doing a quick read through the paper to make sure the text and references are ok.

ivastar commented 11 months ago

@MikeSWang a couple of small corrections:

MikeSWang commented 11 months ago

Thanks @ivastar for the suggestions! I have made these edits and updated the paper in the repo.

MikeSWang commented 11 months ago

@editorialbot generate pdf

editorialbot commented 11 months ago

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

ivastar commented 11 months ago

@editorialbot set 10.5281/zenodo.10072128 as archive

editorialbot commented 11 months ago

Done! archive is now 10.5281/zenodo.10072128

ivastar commented 11 months ago

@editorialbot set v0.3.0 as version

editorialbot commented 11 months ago

Done! version is now v0.3.0

ivastar commented 11 months ago

@editorialbot generate pdf