jose-reviews icon indicating copy to clipboard operation
jose-reviews copied to clipboard

[REVIEW]: AstronomyCalc: A python toolkit for teaching Astronomical Calculations and Data Analysis methods

Open editorialbot opened this issue 1 year ago β€’ 44 comments
trafficstars

Submitting author: @sambit-giri (Sambit Kumar Giri) Repository: https://github.com/sambit-giri/AstronomyCalc Branch with paper.md (empty if default branch): Version: 1.1 Editor: @arm61 Reviewers: @kelle, @AstrobioMike Archive: 10.5281/zenodo.14719952 Paper kind: software

Status

status

Status badge code:

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

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

@kelle & @AstrobioMike, 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://openjournals.readthedocs.io/en/jose/reviewer_guidelines.html. Any questions/concerns please let @arm61 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 @kelle

πŸ“ Checklist for @AstrobioMike

editorialbot avatar Oct 08 '24 10:10 editorialbot

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 avatar Oct 08 '24 10:10 editorialbot

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

βœ… OK DOIs

- 10.1108/20468251211179678 is OK
- 10.1109/CIDU.2012.6382200 is OK
- 10.3847/1538-4357/ac8e04 is OK
- 10.3847/0004-6256/152/6/157 is OK
- 10.1007/978-1-4757-4145-2_7 is OK
- 10.1016/j.physrep.2006.08.002 is OK
- 10.1088/0034-4885/75/8/086901 is OK
- 10.1111/j.1365-2966.2012.21293.x is OK
- 10.1093/mnras/stx2539 is OK
- 10.1093/mnras/stt1341 is OK
- 10.1093/mnras/stv2679 is OK
- 10.1093/mnras/stx649 is OK
- 10.1093/mnras/stz1220 is OK
- 10.1093/mnras/stz2224 is OK
- 10.1088/1475-7516/2019/02/058 is OK
- 10.1093/mnras/sty1786 is OK
- 10.1093/mnras/stv571 is OK
- 10.1088/1538-3873/129/974/045001 is OK
- 10.1007/s10686-013-9334-5 is OK
- 10.1051/0004-6361/201220873 is OK
- 10.1017/pasa.2018.37 is OK
- 10.1017/pasa.2012.007 is OK
- 10.1109/JPROC.2009.2017564 is OK

🟑 SKIP DOIs

- No DOI given, and none found for title: Variation theory and the improvement of teaching a...
- No DOI given, and none found for title: Introduction to cosmology
- No DOI given, and none found for title: Tomographic studies of the 21-cm signal during rei...

❌ MISSING DOIs

- None

❌ INVALID DOIs

- None

editorialbot avatar Oct 08 '24 10:10 editorialbot

Software report:

github.com/AlDanial/cloc v 1.90  T=0.04 s (1058.8 files/s, 237030.5 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Jupyter Notebook                 9              0           6172           2208
Python                          16            219            382            607
TeX                              1             27              0            334
Markdown                         5             61              0            128
YAML                             4             19             26            110
reStructuredText                 9             46             38             95
make                             1              4              7              9
Bourne Shell                     2              6             17              7
-------------------------------------------------------------------------------
SUM:                            47            382           6642           3498
-------------------------------------------------------------------------------

Commit count by author:

   117	sambit-giri
     9	Sambit Kumar Giri

editorialbot avatar Oct 08 '24 10:10 editorialbot

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

editorialbot avatar Oct 08 '24 10:10 editorialbot

Paper file info:

πŸ“„ Wordcount for paper.md is 940

βœ… The paper includes a Statement of need section

editorialbot avatar Oct 08 '24 10:10 editorialbot

License info:

βœ… License found: MIT License (Valid open source OSI approved license)

editorialbot avatar Oct 08 '24 10:10 editorialbot

Review checklist for @kelle

Conflict of interest

Code of Conduct

General checks

  • [x] Repository: Is the source code for this software available at the https://github.com/sambit-giri/AstronomyCalc?
  • [x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • [x] Version: Does the release version given match the GitHub release?
  • [x] Authorship: Has the submitting author (@sambit-giri) made substantial contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • [x] Installation: Does installation proceed as outlined in the documentation? (and documentation is sufficient?)
  • [x] Functionality: Have the functional claims of the software been confirmed?
  • [x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • [x] A statement of need: Do the authors clearly state the need for this software and who the target audience is?
  • [x] Installation instructions: Is there a clearly stated list of dependencies? (Ideally these should be handled with an automated package management solution.)
  • [x] Example usage: Do the authors include examples of how to use the software?
  • [x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • [x] Tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • [x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • [x] Authors: Does the paper.md file include a list of authors with their affiliations?
  • [x] A statement of need: Do the authors clearly state the need for this software and who the target audience is?
  • [x] References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

kelle avatar Oct 08 '24 14:10 kelle

@kelle Thank you for the comments and suggestions in the issues list of the repository. I will start working on them.

sambit-giri avatar Oct 31 '24 13:10 sambit-giri

Hi @kelle,

I have started working on the issues you created.

Most of the issues suggest using another widely-used package for each tutorial notebook instead. While this could help students learn to use these packages, it may not fully achieve our goal of teaching them how to develop and implement these methods from scratch. The primary goal of this package is to demonstrate how the concepts and equations taught in the classroom can be implemented in code. The simple implementations would serve as the starting point that students further develop during the tutorial sessions, with group activities built around this. This approach encourages students to understand the underlying principles and develop their own functions, rather than simply using pre-existing packages.

To balance this, I am considering extending the notebook with advanced examples that utilize widely-used packages. This way, students can first learn the basic methods by coding them from scratch and then explore how these methods are implemented in professional tools. This approach will help them understand both the fundamental concepts and the practical applications. What do you think about this plan?

sambit-giri avatar Dec 09 '24 12:12 sambit-giri

I am still a bit confused about why a package is needed if the goal is to teach them "from scratch", but I see the gist of your point. I think your idea of expanding the notebooks to demonstrate the astropy functionality is an excellent one.

kelle avatar Dec 09 '24 14:12 kelle

The initial modules were created while teaching a bachelor's course introducing students to cosmology. I have found that several similar introductory courses in astronomy do not have basic coding knowledge as a mandatory prerequisite. Therefore, many students in my cosmology course needed some examples to get started. These examples helped them quickly build on their knowledge to solve the assignment questions.

I have tried pointing the students to popular packages, but only some of them could figure it out. I will expand the notebooks introducing other relevant packages (e.g. astropy and emcee) so that the students have the option to learn the advanced tools.

sambit-giri avatar Dec 09 '24 14:12 sambit-giri

Hi @arm61, I have worked on @kelle's comments. I cannot see any report from @AstrobioMike. Please let me know what are the next steps.

sambit-giri avatar Dec 17 '24 08:12 sambit-giri

Review checklist for @AstrobioMike

Conflict of interest

Code of Conduct

General checks

  • [x] Repository: Is the source code for this software available at the https://github.com/sambit-giri/AstronomyCalc?
  • [x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • [x] Version: Does the release version given match the GitHub release?
  • [x] Authorship: Has the submitting author (@sambit-giri) made substantial contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • [x] Installation: Does installation proceed as outlined in the documentation? (and documentation is sufficient?)
  • [x] Functionality: Have the functional claims of the software been confirmed?
  • [x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • [x] A statement of need: Do the authors clearly state the need for this software and who the target audience is?
  • [x] Installation instructions: Is there a clearly stated list of dependencies? (Ideally these should be handled with an automated package management solution.)
  • [x] Example usage: Do the authors include examples of how to use the software?
  • [x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • [x] Tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • [x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • [x] Authors: Does the paper.md file include a list of authors with their affiliations?
  • [x] A statement of need: Do the authors clearly state the need for this software and who the target audience is?
  • [x] References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

AstrobioMike avatar Dec 17 '24 11:12 AstrobioMike

Sorry for my lack of timeliness, @sambit-giri and @arm61.

@sambit-giri, you've done excellent work on this! I don't have any required changes from my point of view. Just some small suggestions below you can feel free to integrate or not. They are short and only a few, so i'm just putting them here rather than as issues in your repo.

@arm61, i support publication and have filled in the checklist πŸ‘

  • Noting what python version is required in the installation text would be useful. Prior to doing the pip install, often users will create a virtual environment (like you suggest in the readthedocs πŸ‘), and knowing the required python base ahead of time can help save time. Adding a line like "If creating a virtual environment, it should use python X.X", or something.

  • I don't know if it would cause problems with other dependencies, but if possible, you might consider adding jupyter to your package dependencies so after users install your package, they can immediately use the notebooks you provide with out needing to figure out installing something else. And/or just making a note somewhere how folks can setup an environment to use the notebooks could help. Though I also want to explicitly acknowledge that you've also already setup google colabs and mybinders, which is great!

  • You might consider linking to the relevant notebooks on the readthedocs tutorial page. E.g., being able to click to download the model_universes.ipynb notebook from the https://astronomycalc.readthedocs.io/en/latest/examples/tutorials.html#Model-universes section would be convenient. Or just linking at the top, e.g., instead of just saying "This is a tutorial to show a usage of the AstronomyCalc package.", you could add to the end, "Corresponding notebooks can be found [here]" (linking-to-the-notebooks-directory).

  • Super-minor text thing: under the "Tests" section of the main README and in the readthedocs, the sentence says to "run either of the following" but only lists one example. And you could also add a note that the user would need to be in the installed AstronomyCalc directory (since not everyone is familiar with how pytest works). (it would have been easier to suggest these text things as a pull request, but i didn't want to be listed as a contributor for just adjusting some wording, haha)

Nice work!

AstrobioMike avatar Dec 21 '24 17:12 AstrobioMike

@AstrobioMike Thank you so much!

* Noting what python version is required in the installation text would be useful. Prior to doing the pip install, often users will create a virtual environment (like you suggest in the readthedocs πŸ‘), and knowing the required python base ahead of time can help save time. Adding a line like "If creating a virtual environment, it should use python X.X", or something.

I have added a list in the installation section of readme, which mentions the python version and a recommendation to use virtual environment.

* I don't know if it would cause problems with other dependencies, but if possible, you might consider adding jupyter to your package dependencies so after users install your package, they can immediately use the notebooks you provide with out needing to figure out installing something else. And/or just making a note somewhere how folks can setup an environment to use the notebooks could help. Though I also want to explicitly acknowledge that you've also already setup google colabs and mybinders, which is great!

This is great suggestion. I have added jupyter to the requirements file. I have added a section called Jupyter Notebook Usage with a detailed instruction.

* You might consider linking to the relevant notebooks on the readthedocs tutorial page. E.g., being able to click to download the model_universes.ipynb notebook from the https://astronomycalc.readthedocs.io/en/latest/examples/tutorials.html#Model-universes section would be convenient. Or just linking at the top, e.g., instead of just saying "This is a tutorial to show a usage of the AstronomyCalc package.", you could add to the end, "Corresponding notebooks can be found [here]" (linking-to-the-notebooks-directory).

I have added the link to the folder containing the notebooks to the top of the tutorial.

* Super-minor text thing: under the "Tests" section of the main README and in the readthedocs, the sentence says to "run _either_ of the following" but only lists one example. And you could also add a note that the user would need to be in the installed AstronomyCalc directory (since not everyone is familiar with how pytest works). (it would have been easier to suggest these text things as a pull request, but i didn't want to be listed as a contributor for just adjusting some wording, haha)

Nice catch! Initially I had nosetest also, but it did not make sense to have multiple options. I have fixed the sentence.

sambit-giri avatar Dec 24 '24 15:12 sambit-giri

@arm61 Please let me know what are the next steps.

sambit-giri avatar Dec 26 '24 13:12 sambit-giri

@editorialbot generate preprint

sambit-giri avatar Jan 09 '25 09:01 sambit-giri

:page_facing_up: Preprint file created: Find it here in the Artifacts list :page_facing_up:

editorialbot avatar Jan 09 '25 09:01 editorialbot

Hello @sambit-giri, here are the things you can ask me to do:


# List all available commands
@editorialbot commands

# Get a list of all editors's GitHub handles
@editorialbot list editors

# Adds a checklist for the reviewer using this command
@editorialbot generate my checklist

# Set a value for branch
@editorialbot set jose-paper as branch

# Run checks and provide information on the repository and the paper file
@editorialbot check repository

# Check the references of the paper for missing DOIs
@editorialbot check references

# Generates the pdf paper
@editorialbot generate pdf

# Generates a LaTeX preprint file
@editorialbot generate preprint

# Get a link to the complete list of reviewers
@editorialbot list reviewers

editorialbot avatar Jan 09 '25 09:01 editorialbot

@editorialbot generate pdf

arm61 avatar Jan 20 '25 22:01 arm61

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

editorialbot avatar Jan 20 '25 22:01 editorialbot

Sorry for the delay @sambit-giri (lots of teaching recently). I was wondering about the "Usage Example"; it leads to a large code block in the paper that is maybe better served in the documentation. Thoughts?

arm61 avatar Jan 20 '25 22:01 arm61

Hi @arm61, no problem at allβ€”I completely understand! I agree that the "Usage Example" section contains a large code block. It is essentially a condensed version of the first tutorial already available in the documentation. If you agree, I can remove this section from the paper.

sambit-giri avatar Jan 21 '25 07:01 sambit-giri

Yes, please.

arm61 avatar Jan 21 '25 15:01 arm61

I have removed the "Usage Example" section.

sambit-giri avatar Jan 21 '25 16:01 sambit-giri

@editorialbot generate pdf

arm61 avatar Jan 22 '25 08:01 arm61

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

editorialbot avatar Jan 22 '25 08:01 editorialbot

Great, the next step is for you to proofread it and ensure there are no typos, etc. Let me know when you are happy with the text.

arm61 avatar Jan 22 '25 09:01 arm61

Hi @arm61, I proofread the draft and added a paragraph at the end of the summary section about the Jupyter notebooks provided in this repository.

sambit-giri avatar Jan 22 '25 11:01 sambit-giri

@editorialbot generate pdf

arm61 avatar Jan 22 '25 12:01 arm61