-
Notifications
You must be signed in to change notification settings - Fork 35
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
Martini #164
Comments
Hello there! Just letting you know we have seen this issue and will be following up with pre-review checks shortly! |
Thank you so much for this submission! It's so exciting to see these Editor in Chief checksHi there! Thank you for submitting your package for pyOpenSci Please check our Python packaging guide for more information on the elements
Editor commentsIn your README can you add a quick installation piece? It doesn't have to be too fancy, something like the # Installation section on the Also, are you able to to add a "version submitted"? I'm assuming it is the latest, but we'll want to know what version the review started at. It's okay if there are releases during the review/the accepted version is different, that is expected as you make edits! |
Also, we have an editor lined up; @hamogu will be the editor lined up for this review 🙌 |
@isabelizimm I added the version submitted. Not sure what you're requesting for the installation information in README - there's a paragraph there already so if something else/different is needed I'm not sure what it is. Or do you not mean the package README? |
Ah, I was specifically looking for the way to pip install martini, a section similar to "Installation" on this package. I do see this command in your installation paragraph, but it's quite easy to miss, so it would be ideal to pull it out into its own line, rather than be part of a larger paragraph. This change is non-blocking to your review, just a note to make it easier to skim through details 😄 |
Editor NoteApril 7th: I've found one reviewer, but I'm still looking for a second reviewer. Since that's more difficult than expected, I figured the first reviewer can get started, so that we can make good use of the extra time and any suggestions/PRs that the first reviewer might have can already be discussed and implemented. Editor response to review:Editor comments👋 Hi @taldcroft and @MicheleDelliVeneri! Thank you for volunteering to review Please fill out our pre-review surveyBefore beginning your review, please fill out our pre-review survey. This helps us improve all aspects of our review and better understand our community. No personal data will be shared from this survey - it will only be used in an aggregated format by our Executive Director to improve our processes and programs.
The following resources will help you complete your review:
Please get in touch with any questions or concerns! Your review is due:Reviewers: @taldcroft and @https://forms.gle/F9mou7S3jhe8DMJ16 |
Just a short note that I'm still looking for a second reviewer for martini. There seems to be a bunching of proposals and proposal review deadlines around this time of year that made several people decline whom I asked. I'm doing my best to find someone and sorry for the delay. |
First look review@kyleaoman - I started on the review of InstallationIn all the places where you specify optional pip dependencies with
This link doesn't exist: https://github.com/kyleaoman/martini/tree/2.0.X. TestsTests are currently failing for me in main and 2.0.10 using Python 3.7:
This might be something as simple as a typo where you want CIThe above test failure highlights a lack of CI since the README badge is showing green for tests. The review criteria for badges in the README is shown here, so these will need to be included.
The current GitHub workflow action which is providing the current |
Hi @taldcroft! Thanks for the initial thoughts. A couple of quick things:
|
@kyleaoman - about the CI, got it. I was a bit hasty but the workflow file name The installation docs say that versions of Python 3.7 and later are supported, so the CI testing should probably include all available versions up through 3.12. It is common policy now for scientific Python packages to follow SPEC 0 — Minimum Supported Dependencies and only support Python versions for 3 years from their release date. You can reduce the version proliferation and CI testing by following that, but that is your choice. |
@kyleaoman just to keep you in the loop. I've just contacted the fifth person as a potential second reviewer. I promise we'll find someone, but I'm sorry it's taking way longer than I expected. |
@hamogu Thanks for the update, and no worries. I originally submitted this as an astropy affiliated package in March 2023 as that process was being re-thought, so I've got the hang of being patient with the process. |
@MicheleDelliVeneri : Thank you for agreeing to be the second reviewer for this package. I've updated all the posts above which have instructions on how the review is supposed to work, but I don't know if github send out notifications when I add a handle by editing, so I'm just writing separately here. Please reach out to me if you have any questions about the review or if I can help in any way. |
@kyleaoman About getting a DOI for the code: While there are several ways of getting snapshots of your code into a permanent archive that issues a DOI, the easiest (and what I use for my own projects) is probably the Github-Zenodo integration. That way, every time you make a "release" on github, it triggers a webhook that makes zenodo pull that specific release into zenodo and issue a corresponding DOI. It also pulls most of the metadata (authors, licence, etc.) automatically. (It sometimes takes up to an hour, so don't worry if you don't see it immediately, and sometimes the metadata is not right, e.g. if you have bot commits to your code, you might only want to have the humans listed a "author" on zenodo, but you can edit the metadata on zenodo later if needed.) Of course, you should do whatever you think works best for your package, I'm just trying to make suggestions to help you make that process easy. |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Readme file requirements
The README should include, from top to bottom:
NOTE: If the README has many more badges, you might want to consider using a table for badges: see this example. Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)
UsabilityReviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole.
Functionality
For packages also submitting to JOSS
Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted. The package contains a
Final approval (post-review)
Estimated hours spent reviewing: Review CommentsFootnotes
|
Got Zenodo automated and put in earlier releases too. There's a known issue where the latest release is flagged incorrectly in Zenodo but that's supposed to resolve when the next release is created, which I'll do at the end of review at the latest. |
Thanks @taldcroft for the review. I think that I've now addressed everything raised:
|
HI @kyleaoman here it is my review, sorry for taking some time. Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Readme file requirements
The README should include, from top to bottom:
NOTE: If the README has many more badges, you might want to consider using a table for badges: see this example. Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)
UsabilityReviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole.
Functionality
For packages also submitting to JOSS
Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted. The package contains a
Final approval (post-review)
Estimated hours spent reviewing: Review CommentsAll green from me, workflows have passed the tests on my local server, and I have run all tests locally. |
@taldcroft Could you have a look and see if all your concerns are addressed and, if so, update your comment above by ticking the "final-approval" box? |
@hamogu - Done! |
@MicheleDelliVeneri I noticed that you did not tick the box for "Package supports modern versions of Python", but I just checked, the tests run with Python version up to 3.12, so I think that's safe! I take the editors privilege to edit your review and tick that box before proceeding. |
🎉 has been approved by pyOpenSci! Thank you for submitting and many thanks to for reviewing this package! 😸 Author Wrap Up TasksThere are a few things left to do to wrap up this submission:
🎉 Congratulations! You are now published with both JOSS and pyOpenSci! 🎉 Editor Final ChecksPlease complete the final steps to wrap up this review. Editor, please do the following:
If you have any feedback for us about the review process please feel free to share it here. We are always looking to improve our process and documentation in the peer-review-guide. |
@kyleaoman Just to confirm: I see you did a very recent release on May15th. Is that the version that includes the responses to @taldcroft comments? Or do you plan to make a new release now that it's accepted? I just want to make sure I'm linking to the correct release number. |
@hamogu release 2.0.12 contains the latest commit so yes, that's up to date. There's also a Zenodo DOI for it. I'll do the JOSS submission and other little to-do items asap, probably tomorrow. Thanks! |
Congratulations! I switched to accepted - just remember to fill out the post-review survey and add a badge pointing to this issue, if you want to. Also, we want to invite you to write a blog post (totally optional) on your package for us to promote your work! if you are interested - here are a few examples of other blog posts: and here is a markdown example that you could use as a guide when creating your post. If you are too busy for this no worries. But if you have time - we'd love to spread the word about your package! Last, a huge thanks to @taldcroft and @MicheleDelliVeneri for their review - without reviewers like you PyOpenSci would not be possible. Also, we want to invite all of you (authors and reviewers) you to join the PyOpenSci slack - it's a very active community (more than astropy!) with lots if technical discussion and people happy to help with packaging and related Python questions. Let me know if you are interested, and we'll send you an invite yo any email address you choose! |
I've completed the post-review survey, added the badge, and just now submitted to JOSS - will come back once that process has run its course :) |
Submitting Author: Kyle Oman (@kyleaoman)
All current maintainers: (@kyleaoman)
Package Name: martini
One-Line Description of Package: MARTINI is a modular package for the creation of synthetic resolved HI line observations (data cubes) of smoothed-particle hydrodynamics simulations of galaxies.
Repository Link: https://github.com/kyleaoman/martini
Version submitted: 2.0.11 (note JOSS paper is in branch joss-paper, and that branch is somewhat behind main & 2.0.11)
Editor: @hamogu
Reviewer 1: @taldcroft
Reviewer 2: @MicheleDelliVeneri
Archive: https://zenodo.org/doi/10.5281/zenodo.11193206
JOSS DOI: https://doi.org/10.21105/joss.06860
Version accepted: 2.0.15
Date accepted (month/day/year): 06/03/2024
Code of Conduct & Commitment to Maintain Package
Description
MARTINI is a modular package for the creation of synthetic resolved HI line observations (data cubes) of smoothed-particle hydrodynamics simulations of galaxies. The various aspects of the mock-observing process are divided logically into sub-modules handling the data cube, source, beam, noise, spectral model and SPH kernel. MARTINI is object-oriented: each sub-module provides a class (or classes) which can be configured as desired. For most sub-modules, base classes are provided to allow for straightforward customization. Instances of each sub-module class are given as parameters to the Martini class; a mock observation is then constructed by calling a handful of functions to execute the desired steps in the mock-observing process.
Scope
Please indicate which category or categories.
Check out our package scope page to learn more about our
scope. (If you are unsure of which category you fit, we suggest you make a pre-submission inquiry):
Domain Specific
Community Partnerships
If your package is associated with an
existing community please check below:
For all submissions, explain how the and why the package falls under the categories you indicated above. In your explanation, please address the following points (briefly, 1-2 sentences for each):
The target audience is research astronomers interested in galaxies (broadly, "extragalactic astronomers") from both the theoretical and observational communities. The package provides a way to transform data products from the theory community (smoothed-particle hydrodynamics based simulations of galaxy formation and evolution) into data products closely resembling the atomic hydrogen signal observed with a radio telescope at 21-cm wavelengths. This enables much more faithful comparisons between theoretical predictions and measurements.
I am not aware of any other actively maintained packages with similar purpose.
@tag
the editor you contacted:N/A (I had previously submitted to astropy under their affiliated package scheme and have been redirected here)
Technical checks
For details about the pyOpenSci packaging requirements, see our packaging guide. Confirm each of the following by checking the box. This package:
Publication Options
JOSS Checks
paper.md
matching JOSS's requirements with a high-level description in the package root or ininst/
.Since I'm coming in through the new astropy route, I had not prepared for JOSS requirements, but was thinking of submitting to JOSS soon anyway. I've now pushed a
paper.md
in thejoss-paper
branch. My package is indexed in the ASCL which in turn is indexed in ADS, but neither of these seems to provide an actual DOI so I will need to look into other repositories (probably Zenodo). As far as I understand submission to JOSS happens after the pyOpenSci review so there is a little bit of time to get this done - I expect to have these two items ticked off by the time the pyOpenSci review process is completed.Note: JOSS accepts our review as theirs. You will NOT need to go through another full review. JOSS will only review your paper.md file. Be sure to link to this pyOpenSci issue when a JOSS issue is opened for your package. Also be sure to tell the JOSS editor that this is a pyOpenSci reviewed package once you reach this step.
Are you OK with Reviewers Submitting Issues and/or pull requests to your Repo Directly?
This option will allow reviewers to open smaller issues that can then be linked to PR's rather than submitting a more dense text based review. It will also allow you to demonstrate addressing the issue via PR links.
Confirm each of the following by checking the box.
Please fill out our survey
submission and improve our peer review process. We will also ask our reviewers
and editors to fill this out.
P.S. Have feedback/comments about our review process? Leave a comment here
Editor and Review Templates
The editor template can be found here.
The review template can be found here.
Footnotes
Please fill out a pre-submission inquiry before submitting a data visualization package. ↩
The text was updated successfully, but these errors were encountered: