karllark / dust_extinction

Astronomical Dust Extinction
http://dust-extinction.readthedocs.io
BSD 3-Clause "New" or "Revised" License
39 stars 23 forks source link

JOSS Review: Instructions on Contributing Curves and Minor Comments #222

Closed TheSkyentist closed 2 months ago

TheSkyentist commented 2 months ago

This is a part of https://github.com/openjournals/joss-reviews/issues/7023

The purpose, documentation, and examples for the code are all well described in the JOSS submission. The code is installable, flexible, and useful for current and future astronomical analysis.

One of the most useful parts is the vast array of extinction models available to the end user, with the ability for contributing more as stated as one of the strengths of the code in the final sentence of the draft. As this is a stated feature of the code:

  1. Would it be possible to add documentation on the format required for an extinction curve that could be merged into the repository?
  2. Or, equally, to add documentation to the meta-classes to enable an end user to easily create "on-the-fly" extinction curves.

I believe that this would satisfy the Documentation Community Guidelines section of the JOSS checklist.

In addition, I have a few minor comments on the draft around language:

  1. Line 6, it might sound better by removing the first instance of word "photon".
  2. Line 30, I believe the units A(lambda) should be explicitly stated here as magnitudes.
  3. Line 36, would it be possible to change "like" to "such as", as the former sounds conversational.
karllark commented 2 months ago

All good suggestions. I will work to get these done ASAP.

karllark commented 2 months ago

For the comments on the text of the paper.

  1. I've not removed the first photon. The reason is that with only the 2nd photon it reads as "absorbing photons out of the line-of-sight." This is not correct. I agree that it is a bit awkward, but it is more accurate.
  2. Done.
  3. Done.
karllark commented 2 months ago

If you find that #224 does not address this issue fully, please reopen this issue (once it is closed by merging the PR).