Open editorialbot opened 3 months ago
editorialbot
commented
3 months 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 pdfReference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1016/j.physrep.2018.11.006 is OK
- 10.1088/1475-7516/2014/05/015 is OK
- 10.48550/arXiv.2206.14320 is OK
- 10.21105/joss.03703 is OK
- 10.1088/1475-7516/2022/11/050 is OK
- 10.1103/physrevlett.129.151102 is OK
- 10.1103/physrevlett.129.151103 is OK
- 10.1103/PhysRevD.107.104036 is OK
- 10.1103/physrevd.108.044022 is OK
- 10.1016/j.dark.2021.100819 is OK
- 10.1103/PhysRevD.107.123535 is OK
- 10.1088/1475-7516/2022/03/053 is OK
- 10.1088/0264-9381/32/23/234003 is OK
- 10.1007/978-3-642-24525-1 is OK
- 10.1088/0264-9381/32/24/245011 is OK
- 10.21105/joss.05956 is OK
- 10.5281/zenodo.10380404 is OK
- 10.1007/3-540-36569-9_13 is OK
- 10.1016/j.cpc.2006.02.002 is OK
- 10.1103/PhysRevD.76.104015 is OK
- 10.5281/zenodo.3565474 is OK
- 10.1103/PhysRevD.77.024027 is OK
- 10.1103/PhysRevD.82.024005 is OK
- 10.1103/PhysRevD.85.124010 is OK
- 10.1142/9789812834300_0200 is OK
- 10.1016/S0010-4655(02)00847-0 is OK
- 10.1103/PhysRevD.93.063006 is OK
- 10.5281/zenodo.4734670 is OK
- 10.1016/j.jcp.2016.12.059 is OK
- 10.1142/S0218271819500147 is OK
- 10.1103/PhysRevD.97.064036 is OK
- 10.1103/PhysRevD.93.124059 is OK
- 10.1088/1475-7516/2020/01/007 is OK
- 10.1088/1361-6382/aad7f6 is OK
- 10.3847/1538-4365/ac157b is OK
- 10.1201/b12985 is OK
- 10.48550/arXiv.2205.07784 is OK
- 10.1109/MCSE.2007.55 is OK
- 10.1016/j.physletb.2016.07.052 is OK
- 10.1016/j.physletb.2016.04.017 is OK
- 10.1088/1475-7516/2016/02/004 is OK
- 10.1007/JHEP04(2014)067 is OK
- 10.1088/1475-7516/2016/06/048 is OK
- 10.1103/PhysRevD.94.044024 is OK
- 10.1103/PhysRevD.93.104016 is OK
- 10.1088/1475-7516/2017/08/024 is OK
- 10.1103/PhysRevD.96.084049 is OK
- 10.1103/PhysRevD.95.123540 is OK
MISSING DOIs
- No DOI given, and none found for title: Chombo software package for AMR applications - des...
INVALID DOIs
- None
Software report:
github.com/AlDanial/cloc v 1.90 T=0.16 s (2184.7 files/s, 290874.1 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
C/C++ Header 271 5245 5229 23499
C++ 35 1184 1003 7367
Python 3 181 140 621
TeX 1 0 0 600
make 29 224 143 376
YAML 6 52 11 324
INI 3 53 65 165
Markdown 2 40 0 78
-------------------------------------------------------------------------------
SUM: 350 6979 6591 33030
-------------------------------------------------------------------------------Commit count by author:
6 Shaun Fell
5 Shaun-F
1 ShaunFell
1 dependabot[bot]Paper file info:
📄 Wordcount for paper.md is 1810
✅ The paper includes a Statement of need section
editorialbot
commented
3 months ago License info:
✅ License found: BSD 3-Clause "New" or "Revised" License (Valid open source OSI approved license)
editorialbot
commented
3 months ago :point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Sbozzolo
commented
3 months ago Sbozzolo
commented
3 months ago Hi @ShaunFell,
I started my review and everything is looking great.
Could you please clearify the relationship between GRBoondi and GRChombo?
In terms of functionalities provided, my first impression is that GRBoondi would be optimally suited to be a module for GRChombo instead of being a separate codebase (GRBoondi even comes with a complete copy of GRChombo). Is there any intention to make GRBoondi an official module for GRChombo? Or, should GRBoondi be considered a hard fork of GRChombo? Having GRBoondi as a module would help with shared tooling (e.g. post-processing) and reduce the maintenance burden on your end.
It would be really useful to have an end-to-end tutorial that explains how to run a simple example GRBoondi. Right now, most of the "getting started" is spread across multiple wiki pages and it is a little intimidating to figure out how to run a simple case.
I'd suggest adding a page with a tutorial to run a simple example. In this tutorial, you can focus on just running something and making a simple visualization. Then, you can provide backlinks to the pages on "Parameter files", "Running Examples", "Visualization" for more in depth discussions.
Could you also elaborate on the differences with GRDzhadzha?
KAClough
commented
3 months ago Hi @ShaunFell I haven't had a chance to go through the code but I wanted to echo what @Sbozzolo said about the relationship between GRChombo and GRBoondi not being very clear.
I would strongly suggest that you don't duplicate code from the GRChombo or GRDzhadzha repositories - it is fine for this code to have dependencies on them, in the same way as GRDzhadzha does on GRChombo, and this allows updates and optimisations that we make there to flow into your code. If there are aspects you found you needed to change in those repositories, I'd be happy to discuss making changes to them there to ensure compatibility, or finding a workaround.
If you agree I would suggest that this is amended before we review the code in more detail.
ShaunFell
commented
3 months ago Hi @KAClough! Ok, yea I originally included the GRChombo source because I needed to modify the level factory class, but I ended up implementing my own, so removing GRChombo in the source and instead using environment variables to point to the source isnt really an issue for me. I will remove the source from the repo and update the main branch accordingly. Thank you for the suggestion!
ShaunFell
commented
3 months ago Hi @Sbozzolo! Thank you for your feedback. I will go through the issues in the repo and respond accordingly in the next week or so. Just for transparency, I am writing my thesis, so my responses will be longer than usual.
Could you please clearify the relationship between GRBoondi and GRChombo?
GRBoondi utilizes a lot of the source code from GRChombo, but also defines it's own functions for specifying the physical problems to that of generalized Proca around arbitrary spacetimes. While GRBoondi utilizes a lot of the GRChombo source, like the AMR interpolator and level classes, it also defines many of its own functionality, such as BaseProcaFieldLevel, since generalized Proca problems typically all share very similar structures when it comes to the code, like computing surface and volume integrals of the Proca norm, the Eulerian energy, etc. So broadly speaking, GRBoondi is very much it's own program that uses the AMR, level class, boundary code, etc. functionality of GRChombo/Chombo.
In terms of functionalities provided, my first impression is that GRBoondi would be optimally suited to be a module for GRChombo instead of being a separate codebase (GRBoondi even comes with a complete copy of GRChombo). Is there any intention to make GRBoondi an official module for GRChombo? Or, should GRBoondi be considered a hard fork of GRChombo? Having GRBoondi as a module would help with shared tooling (e.g. post-processing) and reduce the maintenance burden on your end.
Sort of extending on what @KAClough mentioned, the GRChombo source will be removed from the GRBoondi source code. I did this since I had to modify some of the source code, but ended up creating my own classes and just left GRChombo in the GRBoondi source. I will remove this shortly with a pull request. So GRBoondi will required a fork of GRChombo. At the moment, I have no intention of making GRBoondi a module of GRChombo, though thats an interesting idea! generalized Proca theories are quite complex so keeping GRBoondi separate from GRChombo allows me to be quite versatile in changing the code for the needs of higher-order theories in the generalized Proca landscape.
It would be really useful to have an end-to-end tutorial that explains how to run a simple example GRBoondi. Right now, most of the "getting started" is spread across multiple wiki pages and it is a little intimidating to figure out how to run a simple case.
ok, great idea! I will implement such a tutorial then.
I'd suggest adding a page with a tutorial to run a simple example. In this tutorial, you can focus on just running something and making a simple visualization. Then, you can provide backlinks to the pages on "Parameter files", "Running Examples", "Visualization" for more in depth discussions.
Great idea, thanks!
Could you also elaborate on the differences with
GRDzhadzha?
GRDzhadzha is quite a generic toolkit for simulating any sort of matter field on generic fixed spacetimes. If one wants to simulate generalized Proca, they run into the same sort of problem as if they were to use GRChombo. Besides the lack of post-processing tools, users would have to manually rewrite a lot of the boilerplate code for each generalized Proca theory they simulate. This is basically the whole reason GRBoondi was created, to simplify a lot of this boilerplate code, making it much easier for users to study the generalized Proca landscape. For example, BaseProcaFieldLevel contains almost all the level class code that is common to all generalized Proca theories. This motif is shared throughout many of the classes in GRBoondi.
ShaunFell
commented
3 months ago @KAClough I've removed the GRChombo source code in https://github.com/ShaunFell/GRBoondi/pull/15 and updated the main branch
ShaunFell
commented
3 months ago @editorialbot check repository
editorialbot
commented
3 months ago Software report:
github.com/AlDanial/cloc v 1.90 T=0.07 s (1960.2 files/s, 244355.1 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
C/C++ Header 95 1746 1762 7257
C++ 9 349 261 2031
Python 3 181 140 621
TeX 1 0 0 600
YAML 6 54 11 338
make 10 84 57 188
INI 3 53 65 165
Markdown 2 40 0 78
-------------------------------------------------------------------------------
SUM: 129 2507 2296 11278
-------------------------------------------------------------------------------Commit count by author:
7 Shaun Fell
6 Shaun-F
1 ShaunFell
1 dependabot[bot]Paper file info:
📄 Wordcount for paper.md is 1810
✅ The paper includes a Statement of need section
editorialbot
commented
3 months ago License info:
✅ License found: BSD 3-Clause "New" or "Revised" License (Valid open source OSI approved license)
KAClough
commented
3 months ago Thanks @ShaunFell for the update. I'm very happy you can easily remove the copy of GRChombo!
On GRDzhadzha, there is still a reasonable amount of duplication of core code from that in your repo... I don't feel as strongly that you should inherit from that code and I understand the desire to reduce dependencies, but if you were to link to it then any future tools we introduce there, like new fixed backgrounds or diagnostics (of which you already directly use some), could naturally flow into your code.
I think as a minimum the README should acknowledge the use of parts of the GRDzhadzha code and the documentation should make clear the rationale for not inheriting from it.
Sbozzolo
commented
2 months ago Hi @ShaunFell, I agree with @KAClough. I think that it would be a net positive for everyone to work more closely with the GRChombo collaboration and re-use and share as much code as possible.
In any case, it should be very clear how GRChombo is used. As it stands, any update to GRChombo might end up breaking GRBoondi.
Proca theories are quite complex so keeping GRBoondi separate from GRChombo allows me to be quite versatile in changing the code for the needs of higher-order theories in the generalized Proca landscape.
Choosing to move away from the released GRChombo has implications for the maintenance burden, so users should be provided with this information.
ShaunFell
commented
2 months ago Hi @KAClough! I decided against making GRDzhadzha a dependency since that would add to the large dependency list and make downloading GRBoondi a bit more involved. I only use a very small portion of the GRDzhadzha codebase and I modify some of the files myself, so including it instead as a dependency is a bit unnecessary. I added this mention to the GRBoondi wiki.
@Sbozzolo, I updated GRBoondi to make GRChombo a dependency, so updates to GRChombo now naturally flow into GRBoondi.
KAClough
commented
1 month ago Hi @ShaunFell I see why you did it like this but having given this some more thought I do feel that GRDzhadzha should be a dependency. You currently don't use dynamical NR backgrounds (but I expect you may want to extend to that eventually), you use fixed backgrounds, which is very much a feature of GRDzhadzha and not GRChombo. Almost all your background metric code and the associated tests are directly taken from GRDzhadzha. We are currently developing a number of new background metrics that you would be able to take advantage of if you integrated with the GRDzhadzha code, whereas as it stands you would need to copy them over manually. Equally, we could benefit from the KerrdeSitter background if you separated it out and added it to GRDzhadzha rather than having it in this repo.
Realistically, your target market for this code is going to be GRChombo users, and if I were advising a student of mine who wanted to use non linear Proca-like matter, I would tell them to just copy over specific files from your repo to GRDzhadzha and fix any dependencies, rather than downloading your repo with so much duplication built into it. It would be quite annoying to have to check what, if any, changes you had made in all the similar looking files like LinearMomConservation.hpp for example.
In summary, I think you should remove all duplication, separate out general tools that could be useful to others using fixed backgrounds with different matter and add them to GRDzhadzha, and then focus this code repository on the specific Proca examples and diagnostics that you have developed so it is really clear where the boundary lies between the codes. This will also help if you do want to add NR backgrounds in future, as the background will always be provided by another code, and yours will sit clearly on top and interface with either.
If it helps we can discuss this in a zoom call some time, maybe with @Sbozzolo if he agrees?
ShaunFell
commented
1 month ago Hi @KAClough! Thanks for your comments! I think a Zoom call would be great. I definitely see the excellent points in making GRDzhadzha a dependency and separating out things from my code, but I've made so many modifications to those individual files, it was just easier to modify each file than make GRDzhadzha an extra dependency and creating new classes that overloaded those functions. But we can discuss these things over a Zoom call!
Sbozzolo
commented
1 month ago Hi @KAClough and @ShaunFell.
I cannot speak of the technical details and code provenance, but from the advertised capabilities, my impression is that the community would be best served if GRBoondi were a module of GRChombo (or GRDzhadzha) instead of a separated package. My primary concerns are fair credit attribution, maintenance burden, and alignment of development efforts. If this is not possible, I think that making GRDzhadzha a dependency is a choice that also depends on @ShaunFell’s future plans for GRBoondi.
Happy to have a conversation about this on Zoom!
KAClough
commented
1 month ago Thanks @Sbozzolo! Would 5pm UK time tomorrow work for a call? I think you are both in the states. Otherwise 6pm UK time on Wednesday?
Sbozzolo
commented
1 month ago Assuming "tomorrow" is Tuesday 20 August, yes, 5 PM UK time (9 AM Pacific Time) would work for me.
ShaunFell
commented
1 month ago That works for me! Im on Mountain time right now so that's not too early.
KAClough
commented
1 month ago Ok, here is a zoom for tomorrow:
Topic: Katy Clough's Zoom Meeting Time: Aug 20, 2024 05:00 PM London
Join Zoom Meeting https://qmul-ac-uk.zoom.us/j/81262246232
Meeting ID: 812 6224 6232
ShaunFell
commented
1 month ago Great, thanks Katy!
xuanxu
commented
3 weeks ago @ShaunFell can you update shortly on how is this progressing?, did you all have a productive call?
ShaunFell
commented
3 weeks ago Hi @xuanxu!
Yes, we all had a very productive meeting. Katy and I will be at a numerical relativity meeting next week and will discuss some finer details of the meeting points.
I just submitted my thesis last Thursday so haven't had any time to work on this submission, but I will starting in a few days again.
xuanxu
commented
5 days ago Hi @ShaunFell! is there any progress here? If you need time I can pause the review until you can return to the submission. Otherwise, please update here even with small changes.
ShaunFell
commented
5 days ago Hi @xuanxu! At this point, I am waiting on @KAClough review comments. We spoke at the conference and she mentioned she would be busy the past two weeks but should have time this week.
KAClough
commented
5 days ago Yes, apologies, this is on me to review. As Shaun says I should have time this week, or early next at the latest.
Submitting author: !--author-handle-->@ShaunFell<!--end-author-handle-- (Shaun D. B. Fell) Repository: https://github.com/ShaunFell/GRBoondi Branch with paper.md (empty if default branch): paper Version: v1.0.0 Editor: !--editor-->@xuanxu<!--end-editor-- Reviewers: @Sbozzolo, @KAClough 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
@Sbozzolo & @KAClough, 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 @xuanxu 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 @Sbozzolo