search

openjournals / joss-reviews

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

[REVIEW]: Orbital Dynamics in X-ray stellar binary systems #7220

Open editorialbot opened 2 months ago

editorialbot commented 2 months ago

Submitting author: !--author-handle-->@xragua<!--end-author-handle-- (Graciela Sanjurjo Ferrín) Repository: https://github.com/xragua/xraybinaryorbit Branch with paper.md (empty if default branch): Version: v1 Editor: !--editor-->@warrickball<!--end-editor-- Reviewers: @matteobachetti, @renecotyfanboy Archive: Pending

Status

status

Status badge code:

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

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

@matteobachetti & @tddesjardins, 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 @warrickball 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 @matteobachetti

📝 Checklist for @renecotyfanboy

editorialbot commented 2 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 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 2 months ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

✅ OK DOIs

- 10.1162/EVCO_r_00180 is OK
- 10.1093/mnras/staa3953 is OK
- 10.1117/12.2232432 is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.1086/153315 is OK
- 10.1086/160554 is OK
- 10.1103/PhysRevLett.119.161101 is OK
- 10.1093/mnras/stac352 is OK
- 10.48550/arXiv.2202.05399 is OK

🟡 SKIP DOIs

- No DOI given, and none found for title: PySwarms: A Python-Based Swarm Optimization Librar...
- No DOI given, and none found for title: Accretion Power in Astrophysics
- No DOI given, and none found for title: An introduction to modern astrophysics and cosmolo...
- No DOI given, and none found for title: Accretion Power in Astrophysics

❌ MISSING DOIs

- None

❌ INVALID DOIs

- None
editorialbot commented 2 months ago

Software report:

github.com/AlDanial/cloc v 1.90  T=0.08 s (590.1 files/s, 226513.8 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                           6           1965           1516           3635
SVG                              1              0              0           2671
HTML                             7            147             21           1479
Markdown                         3            306              0           1029
JavaScript                      12            138            228            912
CSS                              4            190             35            780
reStructuredText                 5            187             21            535
TeX                              1             21              0            151
Jupyter Notebook                 6              0           2245            136
DOS Batch                        1              8              1             26
YAML                             1              1              4             18
make                             1              4              7              9
-------------------------------------------------------------------------------
SUM:                            48           2967           4078          11381
-------------------------------------------------------------------------------

Commit count by author:

   148  LAEX
     7  GracielaSanjurjoFerrin
editorialbot commented 2 months ago

Paper file info:

📄 Wordcount for paper.md is 1245

✅ The paper includes a Statement of need section

editorialbot commented 2 months ago

License info:

✅ License found: MIT License (Valid open source OSI approved license)

editorialbot commented 2 months ago

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

warrickball commented 2 months ago

Hi @matteobachetti &, @tddesjardins, and thanks again for agreeing to review. This is the review thread for the paper. All of our correspondence will now happen here.

Please read the "Reviewer instructions & questions" in the first comment above, and generate your checklists by commenting @editorialbot generate my checklist on this issue. As you go over the submission, please check off any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines.

The JOSS review is different from most other journals. We aim to work with the authors to help them meet our criteria instead of merely passing judgement on the submission. We also encourage reviewers to submit issues and pull requests on the software repository. When doing so, please mention openjournals/joss-reviews#7220 so that the issue/PR is linked to this thread. Please also feel free to comment and ask questions on this thread. JOSS editors have found it better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

We aim for the review process to be completed within about 4 weeks but start whenever you can. JOSS reviews are iterative and the authors can start responding while you continue to review other parts of the submission.

If it suits your workflow, you're welcome to assign yourself to this issue in the GitHub UI.

Finally, don't hesitate to ask any questions you might have about the process.

matteobachetti commented 2 months ago

Review checklist for @matteobachetti

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

image

Software paper

xragua commented 2 months ago

Dear @matteobachetti and @tddesjardins, Thank you so much for agreeing to review our project! I look forward to receiving your comments and corrections.

warrickball commented 1 month ago

Hi @matteobachetti & @tddesjardins, I was wondering if you'd made any progress with your reviews? We encourage reviewers to comment on whichever aspects they have a moment for at any time, rather than waiting to deliver a single, monolithic review. That way authors can start working on changes while the rest of the review proceeds.

You can also raise issues (or pull requests) in the software repo but try to mention openjournals/joss-reviews#7220 so that the activity is linked here.

matteobachetti commented 1 month ago

@warrickball: yes, I'm very sorry for this slow progress but I had a couple of busy weeks coming back from vacation. I'm playing with the software

tddesjardins commented 1 month ago

@warrickball I'm hoping to work on this today! We're hosting a two-day workshop, and today is day 2...so it was crazy in the lead up last week.

warrickball commented 1 month ago

Hi @matteobachetti & @tddesjardins, I was wondering if you'd made any progress with your reviews since I last checked in? I see there's been some activity in some issues but @xragua has responded in each case (and some can possibly even be closed).

matteobachetti commented 1 month ago

@warrickball @xragua I think I'm done. Sorry for the long wait.

xragua commented 1 month ago

Dear @matteobachetti and @tddesjardins, I just wanted to mention that I have added a small function that fits well this package (density_and_ionization_orbital_phase_theoretical), its purpose is to calculate the density and ionization parameter the radiation emitted by the NS finds from the point that is emitted in the orbit in its path to the observer until certain extent (default 10 R*). It has been added in the documentation, in the package of course, added to the CI tests and in the examples section. Thank you!

xragua commented 1 month ago

Dear @matteobachetti and @tddesjardins, I added MKdocs gh page describing the package, https://xragua.github.io/xraybinaryorbit/ , Thankyou!

matteobachetti commented 1 month ago

@xragua thanks. As I mentioned in my review, I think the docs should do some more than a list of functions with their API documentation. The documentation of a package of this kind, in my opinion, should introduce curious users to the kind of science analysis they can do with it, with a robust introduction (sort of an expanded statement of need!), and then maybe go through a couple of simple examples, the typical "getting started". The API documentation should be for those who are already using the code and want to understand the subtleties of it, at the end. You have good notebooks, so I think it shouldn't be so difficult linking them in the documentation through nbsphinx and form a large section of examples, maybe ordered from the simplest to the most advanced

One of the many examples: https://astropy-regions.readthedocs.io/en/stable/index.html

xragua commented 1 month ago

Dear @matteobachetti, I tried to follow your indications both in the documentation pages, the README and in the paper, I hope that now it looks better, let me know what you think, Thanks a lot!

warrickball commented 1 month ago

@tddesjardins — Have you had a chance to start a review? I noticed you haven't generated a checklist yet.

@matteobachetti — Thanks for your review. You haven't ticked off all the items so I presume you're waiting for further responses from @xragua. Once your satisfied on any given point, please tick off each item, and I suggest also saying you're happy in this comment thread.

@xragua — Note that @matteobachetti has left comments by editing his review checklist. You should address the comments he's raised there.

matteobachetti commented 1 month ago

@xragua the docs look much better now! Great job! Small nitpick: there is a typo in the big all-caps title "GUETTING STARTED"

xragua commented 1 month ago

@matteobachetti, @tddesjardins and @warrickball, I have made the following updates to the project: Modified the paper, pages, and README files. Added coverage calculation in the CI test. *Included a few lines in the community guidelines. I look forward to your feedback. Thank you!

matteobachetti commented 1 month ago

@editorialbot generate pdf

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:

matteobachetti commented 4 weeks ago

@xragua: thanks for all the work you are doing to address my comments. Besides all the nitpicks I'm doing through Issues, I do have one general suggestion that I make in this context though: all the testing/coverage apparatus I've been recommending is incredibly useful, but it's just esthetics (cute green ticks in a checklist) if you commit to main without checking them. Developers of robust open source projects work in branches/forks, test locally, and when code is ready, they do a pull request, during which tests are automatically executed by Github Actions or similar systems on multiple python versions and architectures, coverage is updated, and so on. It seems like a needless burden, but it's very effective in finding code breaks before the code goes into production.

xragua commented 4 weeks ago

@matteobachetti, thanks for the suggestion, I will work like this from now (I did it now when I changed the CI test to use codecov). I really appreciate the guidance, thank you!

warrickball commented 1 week ago

Just to keep everyone in the loop, I'm trying to find a fresh reviewer for this submission but haven't had any positive replies yet.

warrickball commented 1 week ago

@editorialbot check repository

editorialbot commented 1 week ago

Software report:

github.com/AlDanial/cloc v 1.90  T=0.23 s (255.6 files/s, 97561.7 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
HTML                            12            908             66           7718
SVG                              1              0              0           2671
Python                          14           1230           1886           2123
Markdown                         6            283              0            640
Jupyter Notebook                13              0           3834            452
TeX                              1             28              0            285
YAML                             4             35             13            115
CSS                              3             20            100             93
JavaScript                       4              1             11              4
XML                              1              0              0              3
-------------------------------------------------------------------------------
SUM:                            59           2505           5910          14104
-------------------------------------------------------------------------------

Commit count by author:

   250  LAEX
    87  GracielaSanjurjoFerrin
     3  Matteo Bachetti
editorialbot commented 1 week ago

Paper file info:

📄 Wordcount for paper.md is 1131

✅ The paper includes a Statement of need section

editorialbot commented 1 week ago

License info:

✅ License found: MIT License (Valid open source OSI approved license)

matteobachetti commented 1 week ago

@editorialbot generate pdf

editorialbot commented 1 week ago

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

matteobachetti commented 1 week ago

@warrickball @xragua I just realized I had not followed up here. The paper has improved a lot, and I have no major comments left, just one of personal taste: for what I could interpret of the statement of need, I think it should just contain the paragraph about the other software packages, while what is currently in the statement of need would fit perfectly in the general description. But it's a matter of personal taste. Thanks @xragua for addressing my comments.

xragua commented 1 week ago

@matteobachetti, I changed a little bit the structure of the paper as suggested, I think that now looks better and makes more sense. Thank you very much!

warrickball commented 1 week ago

@editorialbot remove @tddesjardins from reviewers

editorialbot commented 1 week ago

@tddesjardins removed from the reviewers list!

warrickball commented 1 week ago

@editorialbot add @renecotyfanboy to reviewers

editorialbot commented 1 week ago

@renecotyfanboy added to the reviewers list!

warrickball commented 1 week ago

@matteobachetti — Thanks! If that concludes your review, would you mind ticking off the remaining items in your review checklist? That'll help make it clear that you've looked at all the items. Thanks!

warrickball commented 1 week ago

Hi @renecotyfanboy, and thanks again for agreeing to review. This is the review thread for the paper. All of our correspondence will now happen here.

Please read the "Reviewer instructions & questions" in the first comment above, and generate your checklist by commenting @editorialbot generate my checklist on this issue. As you go over the submission, please check off any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines.

The JOSS review is different from most other journals. We aim to work with the authors to help them meet our criteria instead of merely passing judgement on the submission. We also encourage reviewers to submit issues and pull requests on the software repository. When doing so, please mention openjournals/joss-reviews#7220 so that the issue/PR is linked to this thread. Please also feel free to comment and ask questions on this thread.

This paper has been delayed by a reviewer having to withdraw so it'd be great if you could start as soon as you can. JOSS reviews are iterative—rather than monolithic—and the authors can start responding to any comments you raise while you continue to review other parts of the submission.

Don't hesitate to ask any questions you might have about the process.

renecotyfanboy commented 4 days ago

Review checklist for @renecotyfanboy

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

renecotyfanboy commented 4 days ago

@editorialbot generate pdf

editorialbot commented 4 days ago

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

renecotyfanboy commented 4 days ago

I am not sure where to comment this, but my review was pretty fast since a lot of work has already been put into this repo. I opened two very minor issues (xragua/xraybinaryorbit#14, xragua/xraybinaryorbit#15) and will wait for the second one to run the examples on my computer.

Also, I see you reference jaxspec in your paper, we now have a proper reference 😉

Now waiting for @xragua

Some very broad/general extra suggestions that are not blocking

These are all suggestions, do whatever you want with this, there's no obligation whatsoever.

xragua commented 4 days ago

@renecotyfanboy thank you so much for agreeing to review this project! :)

I’m currently working on the corrections, though progress has been a bit slower than I would like, as I’m attending a workshop this week. Nevertheless, I will try to address at least all the issues within the next few days.

Regarding the examples: the input parameters are stored in a .txt file in the same folder as the Jupyter notebooks, which are read directly when the function is set to load_directly=True. If load_directly=False, the parameters are uploaded through the form. If you download this .txt file to the same directory as the notebooks and set load_directly=True everything should work well. We chose this approach because the number of required parameters is quite large, and a form-like input makes it more intuitive and easier to experiment and play with different values. However, I understand that this may not always be the most convenient approach. I'm currently adding extra functionality so the parameters can also be passed directly as a list and displayed for easy verification.

Thank you very much for all the advices! This is quite new to me and I really appreciate all the guidance :) I will write again when I get to finish with the new functionallity.

renecotyfanboy commented 1 day ago

@editorialbot generate pdf

editorialbot commented 1 day ago

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

renecotyfanboy commented 1 day ago

Thanks @xragua for addressing the issues. My review is complete @warrickball

warrickball commented 8 hours ago

Thanks @matteobachetti and @renecotyfanboy! @xragua, that concludes a successful review—congratulations!

Let me now when you made your final changes and we'll start working through a post-review checklist. The main item for you will be to make a new tagged release and upload the archive to an archiving service like Zenodo or Figshare. There are multiple ways of doing it but a common one is to set up a GitHub integration with Zenodo. If you go down this route, I think you need to activate the integration before creating the release, so that it's picked up.

You can then let me know the Zenodo DOI and version number, which I'll add to the JOSS metadata.