Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
editorialbot opened this issue Oct 8, 2024 · 19 comments
Assignees
Labels

Comments

@editorialbot
Copy link
Collaborator

editorialbot commented Oct 8, 2024

Submitting author: @sambit-giri (Sambit Kumar Giri)
Repository: https://github.com/sambit-giri/AstronomyCalc
Branch with paper.md (empty if default branch):
Version: v1.0
Editor: @arm61
Reviewers: @kelle, @AstrobioMike
Archive: Pending
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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

👉📄 Download article proof 📄 View article proof on GitHub 📄 👈

@editorialbot
Copy link
Collaborator Author

Paper file info:

📄 Wordcount for paper.md is 940

✅ The paper includes a Statement of need section

@editorialbot
Copy link
Collaborator Author

License info:

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

@kelle
Copy link

kelle commented Oct 8, 2024

Review checklist for @kelle

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the https://github.com/sambit-giri/AstronomyCalc?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: Does the release version given match the GitHub release?
  • 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

  • Installation: Does installation proceed as outlined in the documentation? (and documentation is sufficient?)
  • Functionality: Have the functional claims of the software been confirmed?
  • 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

  • A statement of need: Do the authors clearly state the need for this software and who the target audience is?
  • Installation instructions: Is there a clearly stated list of dependencies? (Ideally these should be handled with an automated package management solution.)
  • Example usage: Do the authors include examples of how to use the software?
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • 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

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

@sambit-giri
Copy link

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

@sambit-giri
Copy link

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?

@kelle
Copy link

kelle commented Dec 9, 2024

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.

@sambit-giri
Copy link

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
Copy link

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.

@AstrobioMike
Copy link

AstrobioMike commented Dec 17, 2024

Review checklist for @AstrobioMike

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the https://github.com/sambit-giri/AstronomyCalc?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: Does the release version given match the GitHub release?
  • 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

  • Installation: Does installation proceed as outlined in the documentation? (and documentation is sufficient?)
  • Functionality: Have the functional claims of the software been confirmed?
  • 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

  • A statement of need: Do the authors clearly state the need for this software and who the target audience is?
  • Installation instructions: Is there a clearly stated list of dependencies? (Ideally these should be handled with an automated package management solution.)
  • Example usage: Do the authors include examples of how to use the software?
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • 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

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

@AstrobioMike
Copy link

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!

@sambit-giri
Copy link

@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
Copy link

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

@sambit-giri
Copy link

@editorialbot generate preprint

@editorialbot
Copy link
Collaborator Author

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

@editorialbot
Copy link
Collaborator Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants