Open editorialbot opened 1 month ago
editorialbot
commented
1 month 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 commandsFor example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
@editorialbot generate pdfSoftware report:
github.com/AlDanial/cloc v 1.90 T=0.10 s (654.8 files/s, 263000.9 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
Python 34 2093 3435 5435
Jupyter Notebook 16 0 11095 1471
HTML 1 50 5 1468
C 2 82 128 301
Markdown 4 81 0 254
TeX 1 11 0 153
YAML 3 10 24 76
MATLAB 1 75 170 51
Bourne Shell 2 6 2 16
C/C++ Header 1 0 0 12
TOML 1 0 0 6
-------------------------------------------------------------------------------
SUM: 66 2408 14859 9243
-------------------------------------------------------------------------------Commit count by author:
486 Scott Field
103 Vijay Varma
47 Chad Galley
26 Jonathan Blackman
19 sfield17
14 Jooheon
11 Kevin Barkett
8 Tousif Islam
6 Leo C. Stein
4 Duncan Macleod
3 Raffi Enficiaud
1 Alexander Harvey Nitz
1 Dwyer Deighan
1 tausigmaislamReference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1103/PhysRevX.4.031006 is OK
- 10.1103/PhysRevLett.115.121102 is OK
- 10.1088/1361-6382/aa7649 is OK
- 10.1103/PhysRevD.95.104023 is OK
- 10.1103/PhysRevD.99.064045 is OK
- 10.1103/PhysRevD.108.064027 is OK
- 10.1103/PhysRevD.106.044001 is OK
- 10.1103/PhysRevResearch.1.033015 is OK
- 10.1103/PhysRevD.102.024031 is OK
- 10.1103/PhysRevD.101.081502 is OK
- 10.1103/PhysRevD.106.104025 is OK
MISSING DOIs
- No DOI given, and none found for title: Waveform Modelling for the Laser Interferometer Sp...
INVALID DOIs
- None
Paper file info:
📄 Wordcount for paper.md is 1221
✅ The paper includes a Statement of need section
editorialbot
commented
1 month ago License info:
🟡 License found: Other (Check here for OSI approval)
editorialbot
commented
1 month ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
plaplant
commented
1 month ago @GarethCabournDavies @Ceciliogq — 👋 thanks for agreeing to review this submission! When you get a chance, please reply to this thread with the message:
@editorialbot generate my checklistThis will make the checklist of items to work through as part of the review. If you have any questions at all, please let me know!
GarethCabournDavies
commented
1 month ago the license is named as MIT/X consortium, but does not contain the text associated with the X consortium variant of the MIT license This does not invalidate that it is using a OSI-approved license, but is somewhat confusing
The author list and acknowledgements appear to contain everyone who has contributed to the github, scott field is the primary contributor and the second author (vijay varma) is the second on the github list of contributions. The full list of authors seems complete
The software is mature and well-maintained. The software has been and is likely to continue to be regularly cited within the gravitational-wave data analysis field The software and associated methods have been in use for many years and are likely to remain so gwsurrogate is packaged appropriately for a python software, and is available on conda-forge and pypi
The data behind the figure is available at https://github.com/sxs-collaboration/gwsurrogate/blob/joss_paper/tutorial/paper/figs/data.npy, though how that is gained is not explained. As with @Ceciliogq, I don't think this is necessarily original data, and so not a problem
as above
There is not API method documentation that I can see - this means that either it doesn't exist, or it is too hard to find!
pytest and the mentioned test codes have been run and verified
there is a (extensive, which I commend) code of conduct I do not see contribution guidelines Issues are as standard for github, but guidelines on how to use the issues (e.g. issue templates) are not given support is not explained
please compare to other software, particularly the Phenom and SEOB families of waveforms (and their availability)
Overall, I wanted to add that this is a nice piece of software; it has obviously been worked on a lot over many years. There are a couple of minor shortcomings in documentation, but overall this is nice. I see no reason that the review should reject this submission if the minor alterations are made
Ceciliogq
commented
2 weeks ago I am not sure what "original data" means. They show a plot of a Numerical Relativity waveform which is not available, but I do not think that should be a problem.
If by results you mean the plot comparing the surrogate with the simulation, then there is not a clear way to reproduce it since I do not know the parameters of that simulation. However I think there is no need to explicitly check this.
Installing from pip and conda was fine. However, I did not manage to install from source. When importing gwsurrogate I get
cannot import LAL __name__ = gwsurrogate.new.spline_evaluation __package__= gwsurrogate.new Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/home/cecilio/.local/lib/python3.8/site-packages/gwsurrogate/__init__.py", line 73, in <module> from . import surrogate File "/home/cecilio/.local/lib/python3.8/site-packages/gwsurrogate/surrogate.py", line 44, in <module> from gwsurrogate.new.surrogate import ParamDim, ParamSpace File "/home/cecilio/.local/lib/python3.8/site-packages/gwsurrogate/new/surrogate.py", line 43, in <module> from .nodeFunction import NodeFunction File "/home/cecilio/.local/lib/python3.8/site-packages/gwsurrogate/new/nodeFunction.py", line 39, in <module> from gwsurrogate.eval_pysur import evaluate_fit ModuleNotFoundError: No module named 'gwsurrogate.eval_pysur'
The code is well documented, and the help command works well, but I have not seen anything like sphinx doc pages which would be nice.
There are automated tests and instructions how to run them, but I did not manage to make it work. When following the instructions I get
ImportError: cannot import name 'evaluate_fit' from 'gwsurrogate.eval_pysur' (unknown location)
I have not seen these.
I do not see this.
plaplant
commented
1 week ago @Ceciliogq thanks for making progress on the review! Please feel free to make issues on the upstream repository pointing out issues that should be addressed as part of this review. It would be excellent if any issues made reference this JOSS review thread. Let me know if you have questions about how to do this.
@GarethCabournDavies 👋 when you get a chance, please start making your way through the review issues. If you have questions about anything, I'm happy to help out!
GarethCabournDavies
commented
1 week ago Note that in reviewing the conflict of interest statement, I am almost exactly one of the cases where a conflict of interest could be perceived, but is not an issue; many of the authors are also part of the LIGO scientific collaboration, and so have co-authored papers but we have not collaborated directly on any funded work
GarethCabournDavies
commented
5 days ago These are some comments on the installation / basic usage with the tests that I wanted to note
from-source installation needs setuptools explicitly (I didn't install it straight away, and everything seemed fine until I import gwsurrogate in python, when things broke!)
Note that for all cases, I needed to go back and re-install with numpy specified as being <2, which I think is being dealt with in https://github.com/sxs-collaboration/gwsurrogate/issues/51
in pip and conda cases when first running import gwsurrogate I get a warning like:
/home/gareth/miniconda3/envs/joss_gwsurrogate_conda/lib/python3.12/site-packages/gwtools/const.py:52: UserWarning: Wswiglal-redir-stdio:
SWIGLAL standard output/error redirection is enabled in IPython.
This may lead to performance penalties. To disable locally, use:
with lal.no_swig_redirect_standard_output_error():
...
To disable globally, use:
lal.swig_redirect_standard_output_error(False)
Note however that this will likely lead to error messages from
LAL functions being either misdirected or lost when called from
Jupyter notebooks.
To suppress this warning, use:
import warnings
warnings.filterwarnings("ignore", "Wswiglal-redir-stdio")
import lal
import lal
lal.MSUN_SI != MsunNot an error, just a warning, thats fine, but would be better to work on how to get rid of it!
I am not a fan of the following being printed every time
__name__ = gwsurrogate.new.spline_evaluation
__package__= gwsurrogate.newbut if thats how you want to do it, then that's fine
Other minor problems:
Following the example in https://github.com/sxs-collaboration/gwsurrogate/tree/master?tab=readme-ov-file#evaluate-the-surrogate, the dyn object returned is a None. This is at odds with the comment in the example about "do dyn.keys() to see contents"
In https://github.com/sxs-collaboration/gwsurrogate?tab=readme-ov-file#tests, need to add that pytest is required!
plaplant
commented
5 days ago @GarethCabournDavies thanks for your review! I appreciate your acknowledging your affiliation with LIGO. I agree that this does not rise to the level of a conflict of interest, so I am formally waiving this as a potential issue. I also appreciate your making issues on the upstream repository pointing out some potential improvements. This is helpful for bookkeeping the review issues.
@sfield17 both reviewers have taken an initial pass at the software package, and pointed out some things that should be addressed. When you get a chance, please start responding to these issues (either in this thread or in the issues on the main repository, whichever you deem more appropriate). When these issues have been resolved, please respond to this review thread saying so. At that point the reviewers can take another pass to see if they have been addressed to their satisfaction.
Thanks everyone for contributing to this review process! Please let me know if you have any questions.
Submitting author: !--author-handle-->@sfield17<!--end-author-handle-- (Scott Field) Repository: https://github.com/sxs-collaboration/gwsurrogate Branch with paper.md (empty if default branch): joss_paper Version: v1.1.6 Editor: !--editor-->@plaplant<!--end-editor-- Reviewers: @GarethCabournDavies, @Ceciliogq Archive: Pending
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
@GarethCabournDavies & @Ceciliogq, 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 @plaplant 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 @GarethCabournDavies
📝 Checklist for @Ceciliogq