search

openjournals / joss-reviews

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

[REVIEW]: ndcube: Manipulating N-dimensional Astronomical Data in Python #5296

Closed editorialbot closed 1 year ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@DanRyanIrish<!--end-author-handle-- (Daniel Ryan) Repository: https://github.com/sunpy/ndcube Branch with paper.md (empty if default branch): Version: v2.1.3 Editor: !--editor-->@adonath<!--end-editor-- Reviewers: @maxnoe, @timj Archive: 10.5281/zenodo.8126828

Status

status

Status badge code:

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

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

@maxnoe & @timj, 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 @adonath 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 @timj

📝 Checklist for @maxnoe

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.22 s (448.7 files/s, 75260.5 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          57           2392           3361           6650
reStructuredText                22            760            959           1221
YAML                            11             39             43            369
Markdown                         1             31              0            248
TeX                              1             17              0            188
SVG                              1              1              1            187
INI                              1              4              0            123
TOML                             1             14              0             65
JSON                             1              0              0             29
DOS Batch                        1              8              1             26
Bourne Shell                     2              1              0             15
make                             1              4              7              9
-------------------------------------------------------------------------------
SUM:                           100           3271           4372           9130
-------------------------------------------------------------------------------

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

Wordcount for paper.md is 1876

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:

adonath commented 1 year ago

@DanRyanIrish @maxnoe @timj this is the review thread for the paper. All of our communications will happen here from now on.

As a reviewer, the first step is to create a checklist for your review by entering

@editorialbot generate my checklist

as the top of a new comment in this thread.

These checklists contain the JOSS requirements. As you go over the submission, please check any items that you feel have been satisfied. The first comment in this thread also contains links to the JOSS reviewer guidelines.

The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention openjournals/joss-reviews#REVIEW_NUMBER so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

We aim for reviews to be completed within about 2-4 weeks. Please let me know if any of you require some more time. We can also use EditorialBot (our bot) to set automatic reminders if you know you'll be away for a known period of time.

Please feel free to ping me (@adonath) if you have any questions/concerns.

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

OK DOIs

- 10.1051/0004-6361/201322068 is OK
- 10.5281/zenodo.7625637 is OK
- 10.1038/s41586-020-2649-2 is OK
- 10.1109/AERO53065.2022.9843340 is OK

MISSING DOIs

- None

INVALID DOIs

- None
timj commented 1 year ago

Review checklist for @timj

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

timj commented 1 year ago

I assume I can leave a general comment before I check all the boxes since checking boxes seems quite final.

The ndcube looks fantastic and I'm really happy that it is based off NDData and can use gwcs. I think I would like a little more description of the state of the field to be included since it all seems to be wrapped up in a couple of sentences.

I would like a more explicit statement about support for gWCS. When I first read the paper I assumed FITS-WCS was all that was supported and it was only after reading the ndcube docs that I realized that there is full support for gWCS. This is really important since FITS-WCS is very restrictive in what can be supported. For example, one party trick the Starlink AST library has is that you can define an IFU 3D coordinate system on a 2D image (this can not be handled in FITS-WCS at all since it requires discrete jumps in cube planes moving from one pixel to the next) and ask AST to regrid it into a 3D cube and it will do it more or less automatically. Can ndcube do that?

References:

Typo on bottom of first page: "is treats them"

timj commented 1 year ago

Also there is a typo in the name of the package in the main readme ("ncdube"):

https://github.com/sunpy/ndcube/blob/main/README.rst?plain=1#L2

DanRyanIrish commented 1 year ago

Thanks @timj for these comments. I will open a PR to the ndcube joss branch so you can see how I've tried to address your comments. If there's a preferred way of working through these comments, please let me know.

DanRyanIrish commented 1 year ago

@timj I have opened this PR to the ndcube main branch with changes in accordance with your comments. This is still a work in progress and you can see which tasks I've already addressed in the checklist in the PR description. I'll let you know when I think it's ready for further feedback. But in the mean time you can see what I'm doing.

DanRyanIrish commented 1 year ago

...For example, one party trick the Starlink AST library has is that you can define an IFU 3D coordinate system on a 2D image (this can not be handled in FITS-WCS at all since it requires discrete jumps in cube planes moving from one pixel to the next) and ask AST to regrid it into a 3D cube and it will do it more or less automatically. Can ndcube do that?

I think @Cadair may want to comment here. From my understanding of your question, NDCube does not explicitly support this. NDCube does have .reproject_to method that takes a new WCS and reprojects the cube to it. The actual calculation is passed onto the reproject package. So if what you're asking for can already be done by reproject, then NDCube already supports it. If not however, there is no reason that an NDCube-inherited object couldn't add a method that does provide such functionality.

DanRyanIrish commented 1 year ago 
* Would it be possible for the ApJ paper to be uploaded to arXiv so it can get a DOI that can be included here? Does JOSS allow "submitted" papers that may or may not happen?

I don't know JOSS's policy on this. Perhaps @adonath can clarify?

DanRyanIrish commented 1 year ago

@timj Thanks again for your constructive feedback.

With the exception of deciding how best to handle the reference to the ndcube ApJ paper, I have made changes to address all your comments. You can this the changes on the PR mentioned above

adonath commented 1 year ago

I don't know JOSS's policy on this. Perhaps @adonath can clarify?

@DanRyanIrish and @timj The policy by JOSS here is to wait for the AAS DOI. Once the DOI is available it should be added to the YAML header of the paper.md file. The guidelines are documented here https://joss.readthedocs.io/en/latest/editing.html?#handling-of-papers-published-together-with-aas-publishing

I'm currently checking with the JOSS editorial board the policy for the hypothetical case that the paper is not accepted by AAS. I will keep you updated once I know more.

DanRyanIrish commented 1 year ago

@adonath I have received the referee's report on the ApJ paper. It's very positive so there doesn't appear to be any risk of the paper being rejected.

@timj Are you happy that the changes made in my ndcube PR address all your concerns?

Cadair commented 1 year ago

@timj This probably isn't the best place to go into the gritty details on this, but there's no reason why NDCube couldn't support the Startlink AST WCS library directly via the APE 14 API. I would assume that the regrid would be easier using the startlink lib rather than going through reproject but it might work :grinning:

adonath commented 1 year ago

@adonath I have received the referee's report on the ApJ paper. It's very positive so there doesn't appear to be any risk of the paper being rejected.

Excellent!

maxnoe commented 1 year ago

Review checklist for @maxnoe

Conflict of interest

Code of Conduct

General checks

The file is called LICENSE.rst, but does not actually contain any special rest markup, so I guess this is fine.

Functionality

Documentation

Software paper

maxnoe commented 1 year ago

Sorry for being a bit late.

I intentionally didn't read the review comments by @timj , so there might be some duplication.

I found some smaller typos and an issue in the references, for which I opened: https://github.com/sunpy/ndcube/issues/610

I think this is really great and I am looking forward to try to use ndcube also for high energy gamma-ray data, we also have a lot of use-cases where ndcube might be appropriate.

Side note: the WCS API needs an overhaul to be a bit more user friendly. At least I find myself thinking about one axis at the time and having to specify the properties as lists over the axes instead of just having a list of axis definitions is always a bit counter-intuitive.

DanRyanIrish commented 1 year ago

Hi @maxnoe. Thanks for your feedback and for your issue on the ndcube repo. I will incorporate the necessary changes into the same PR I am using to address for @timj 's comments.

Side note: the WCS API needs an overhaul to be a bit more user friendly. At least I find myself thinking about one axis at the time and having to specify the properties as lists over the axes instead of just having a list of axis definitions is always a bit counter-intuitive.

I don't quite follow here. Do you mean the AstroPy WCS API?

maxnoe commented 1 year ago

I don't quite follow here. Do you mean the AstroPy WCS API?

Yes, nothing directly to do with ndcube, you just use the astropy WCS object

DanRyanIrish commented 1 year ago

Yes, nothing directly to do with ndcube, you just use the astropy WCS object

Ah, right. Now I see why you labelled it a side note. :)

DanRyanIrish commented 1 year ago

@timj @maxnoe I have addressed all your comments in sunpy/ndcube#609 and merged it. The updates should now be available in the regenerated PDF below.

DanRyanIrish commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

:warning: An error happened when generating the pdf.

DanRyanIrish 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:

adonath commented 1 year ago

@timj I think there are a few points left unchecked on your list, but I think you finished your review. I this this correct?

@DanRyanIrish Once the JOSS review is complete, I'll pause the issue and we wait until the AAS paper is published. Please keep us up to date.

timj commented 1 year ago

@adonath done.

adonath commented 1 year ago

Thanks @timj!

@DanRyanIrish please also let us know once you have a DOI assigned to the AAS paper.

adonath commented 1 year ago

@DanRyanIrish Any update from AAS?

DanRyanIrish commented 1 year ago

@adonath I am awaiting feedback from co-authors before resubmitting the AAS paper. I will let you here once it's resubmitted.

DanRyanIrish commented 1 year ago

@adonath I am happy to say that the associated ndcube ApJ paper has been accepted and we should receive a DOI soon. Before that can happen though, it seems we need to provide ApJ with this paper's DOI. Can you confirm that it will be: https://doi.org/10.21105.joss.05296 ?

Can you also confirm what the month of this JOSS publication should be which it required for proper citing?

adonath commented 1 year ago

Thanks a lot @DanRyanIrish, that's great news! My understand of the process is that the reference only goes in one direction:the ApJ DOI is cited in the JOSS paper, but no the other way around. However I'll tag the EiC @arfon to confirm.

DanRyanIrish commented 1 year ago

If that is normally the case, perhaps it's required for this paper because I cite the JOSS paper in the ApJ one...?

augustfly commented 1 year ago

@adonath I'm not certain what @arfon will recall, but we def pref bi-directional citing (AAS formally cites JOSS). We also banner the final article with the JOSS DOI. I believe the JOSS DOI is set the moment the review issue is begun (by the GitHub issue #, e.g., 5296).

The AAS DOI will be: 10.3847/1538-4357/ace0bd

adonath commented 1 year ago

Thanks @augustfly for the clarification, this makes sense! I will request to maybe extend the JOSS guidelines for joint submissions (https://joss.readthedocs.io/en/latest/editing.html#handling-of-papers-published-together-with-aas-publishing) to mention and check the bi-directional citing.

@DanRyanIrish Yes, the JOSS DOI should be https://doi.org/10.21105/joss.05296. Please also update the meta data in the paper.md with the new AAS DOI. See https://joss.readthedocs.io/en/latest/editing.html#handling-of-papers-published-together-with-aas-publishing for instructions.

Here you can find an example BibTex from a JOSS paper:

@article{Riley2023,
    title        = {X-PSI: A Python package for neutron star X-ray pulse simulation and inference},
    author       = {Thomas E. Riley and Devarshi Choudhury and Tuomo Salmi and Serena Vinciguerra and Yves Kini and Bas Dorsman and Anna L. Watts and Daniela Huppenkothen and Sebastien Guillot},
    year         = 2023,
    journal      = {Journal of Open Source Software},
    publisher    = {The Open Journal},
    volume       = 8,
    number       = 82,
    pages        = 4977,
    doi          = {10.21105/joss.04977},
    url          = {https://doi.org/10.21105/joss.04977}
}

So this does not mention the month, however a it defines a volume and number. I'm not sure how this information is created. Can you please comment @arfon?

DanRyanIrish commented 1 year ago

@adonath I've opened a PR to the ndcube repo to update the bibtex reference. However, I don't have all the info you asked for.

@augustfly Could you clarify what information we are able to supply at this time? The link should take you directly to the bibtex reference being updated. You can comment here or directly on that PR. Thanks!

arfon commented 1 year ago

So this does not mention the month, however a it defines a volume and number. I'm not sure how this information is created. Can you please comment @arfon?

It's generated by the month and year for when the JOSS paper is published. We're currently in volume 8 (and will be for all of this calendar year), and number (issue) 86 for this month. From July this year we'll be in issue 87, August 88 etc.

Also, the page number is the issue number here (5296).

augustfly commented 1 year ago

@adonath I've opened a PR to the ndcube repo to update the bibtex reference. However, I don't have all the info you asked for.

@augustfly Could you clarify what information we are able to supply at this time? The link should take you directly to the bibtex reference being updated. You can comment here or directly on that PR. Thanks!

@DanRyanIrish I think that JOSS handles this (citing/linking to the AAS DOI) through paper.md instead of the paper's bibliography.

The bibtex example @adonath gave is what we (at that AAS Journals) would need, but actually it's all fine from our end until you get our page proofs to double check things.

So I don't think you need full bibtex for the AAS DOI; only update paper.md with the DOI and journal title and then my read is that you're good to go.

DanRyanIrish commented 1 year ago

Ah OK. Thanks for the clarification @augustfly. @adonath I have added the AAS DOI to the PR and merged it. Please let me know if there's anything further needed.

adonath commented 1 year ago

Thanks @DanRyanIrish! The meta data addition looks correct, so I think we can proceed with the publication.

adonath 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.1051/0004-6361/201322068 is OK
- 10.3847/1538-3881/aabc4f is OK
- 10.3847/1538-4357/ac7c74 is OK
- 10.5281/zenodo.7625637 is OK
- 10.1038/s41586-020-2649-2 is OK
- 10.1109/AERO53065.2022.9843340 is OK
- 10.21105/joss.01832 is OK
- 10.5334/jors.148 is OK

MISSING DOIs

- None

INVALID DOIs

- 10.3847/1538-4357/ace0bd is INVALID
adonath commented 1 year ago

The one invalid DOI is expected...so looks good!

adonath 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:

adonath commented 1 year ago

@DanRyanIrish Here are some final minor comments on the paper from my side:

DanRyanIrish commented 1 year ago

@editorialbot generate pdf