-
-
Notifications
You must be signed in to change notification settings - Fork 39
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]: ysoisochrone: A Python package to estimate masses and ages for YSOs #7493
Comments
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:
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
|
Software report:
Commit count by author:
|
|
Paper file info: 📄 Wordcount for ✅ The paper includes a |
License info: ✅ License found: |
Hi @HeloiseS & @vsquicciarini, and thanks again for agreeing to review (and so quickly!). This is the review thread for the paper. All our correspondence is now intended to happen here but you're welcome to email me with any questions, too. Please read the "Reviewer instructions & questions" in the first comment above, and generate your checklists by commenting The JOSS review is different from most other journals. We aim to work with the authors to help them meet our criteria instead of merely passing judgement on the submission. We also encourage reviewers to submit issues and pull requests on the software repository. When doing so, please mention We aim for the review process to be completed within about 4-6 weeks so start whenever you can. JOSS reviews are iterative—rather than monolithic—and the authors can start responding while you continue to review other parts of the submission. If it suits your workflow, you're welcome to assign yourself to this issue in the GitHub UI. |
Hi @HeloiseS & @vsquicciarini, I just wanted to check in on how your reviews were progressing and if you need anything from me. Note that we find JOSS reviews are most efficient if you start commenting on the submission as you work through the checklist, rather than waiting to accumulate a single, monolithic review, as one would for a traditional journal. |
Hi Warwick - things have been crazy here and I'm just about seeing the end of the tunnel. I'm hoping to start this week 👍 |
Review checklist for @HeloiseSConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
Hi Warrick,
Sorry for the late answer. On my side I have been going through extremely
tough weeks lately because I'm moving to a new country for my new postdoc.
I'm going to start the review at the beginning of next week.
Thanks a lot for the patience.
Best,
Vito
Il gio 5 dic 2024, 20:43 Warrick Ball ***@***.***> ha scritto:
… Hi @HeloiseS <https://github.com/HeloiseS> & @vsquicciarini
<https://github.com/vsquicciarini>, I just wanted to check in on how your
reviews were progressing and if you need anything from me. Note that we
find JOSS reviews are most efficient if you start commenting on the
submission as you work through the checklist, rather than waiting to
accumulate a single, monolithic review, as one would for a traditional
journal.
—
Reply to this email directly, view it on GitHub
<#7493 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AST7CJJKOOBEUDNYQQ3FVJT2EC3HJAVCNFSM6AAAAABR7T5INGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMRRGM2TMNZRGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
(A running list I'll update throughout my review of stuff that's not big enough to be put in an issue) Nice-to-have
|
Comments on the paper
A more general question for @DingshanDeng: do none of the other track fitting algorithm allow you to use your own YSO tracks but with their fitting methods? For example isn't there other open source codes where you could instead add the YSO functionality rather than create a whole new package?
For the paper that's all I have so you can get started on those changes as well. Note to @warrickball - I've reviewed paper and install. Started on tutorials but they are giving me an error so until Issue #6 is sorted I've done pretty much all I can do at this point. If changes get made and I miss it (because GH floods my inbox with many things) feel free to poke me. This is now top of my list to get done and if I take more than 24h to show activity it's because I've missed something :) |
Thanks for reviewing the code and your helpful comments on the package and paper! I will start implementing them later next week. Starting from fixing the bugs. |
Review checklist for @vsquicciariniConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
Hi @DingshanDeng, first of all I would like to apologize with you for the long wait. Being amidst a job change with a spare month between the two contracts, I thought I could have had much more free time than I eventually had. I am truly sorry about that. I have read the paper, the documentation and tested the Jupyter Notebook you provided us with. My overall impression of your work is positive, even though I think that are a few issues to be discussed together. I am going to write down comments related to the paper and the documentation. I would like to keep on testing the code on my own for a couple of additional days before discussing the package from a technical point of view. |
Comments on the paper
On the choice of input observablesAs I mentioned before, I think it is necessary to explain why (L, Teff) were chosen over photometry as input variables. Based on my experience, I consider the latter to be much more reliable than the former because:
|
Some comments on the Jupyter NotebookAs I mentioned above, I may write some additional technical comments on the code itself after testing it more carefully. For the moment being, I just write here some general comments that came to my mind while executing the notebook:
|
Comments on the documentation
|
Hi @DingshanDeng, I see @vsquicciarini and @HeloiseS have both left various comments for you. Are you progressing on resolving these? |
Hi @DingshanDeng! Thanks for making the suggested changes and adding text to explain the purpose and need for the code. |
As a general question, to my eyes it seems from the previous plots that the two theoretical grids, albeit very close, do not exactly overlap. Is that true?
|
I've tried to run a test for a star with a temperature close to 3900 K to investigate what happens. These are my input parameters: and here is the HRD: I verified empirically that the star is just fitted with the Baraffe isochrones (the posteriors look the same). This means that the whole grid, both hotter and colder than 3900 K, is set equal to Baraffe's grid. But should not the program compare data to a blend of Baraffe (T_eff<3900 K) and Feiden (T_eff>3900 K) isochrones? There is a second problem. There is a sharp cut at log(t)~7.7 (t~50 Myr) in the posteriors, meaning that solutions with t>50 Myr are not considered. Why does this happen? Is there some hidden cropping of the grid that discards everything older than 50 Myr? Notice that this happens when selecting Feiden's tracks too: (P.S.: I suggest hiding the printing of RuntimeWarnings here, if you think that they are being properly handled) |
I have no other comment :) |
Thanks for the reminder. I have not updated the pip version. I will update that once I finished the tests/comments suggested above :-). |
@editorialbot generate pdf |
Hi @DingshanDeng, sorry to get back to you so late. Thanks for the time you took to implement these changes and to clarify the points raised by some of my remarks. I do not have any other comment to make. Green light on my side! |
Hi all, Thanks again for your helpful comments and suggestions. I just released the first production ready version 1.0.0 to PyPI and GitHub. We hope this package/tool could be useful to the whole community. Hi @warrickball, is there any other thing that we need to go through or edit for the publication? If so, please let me know, and I should be able to address them this week. Best |
I'd just like to check in with @HeloiseS, who wrote
I see that you've ticked everything off in the review checklist. Have you run everything you wanted to and, if so, can you confirm you're happy to recommend the paper be accepted? Thanks! |
Yes all good!!! |
Great! In that case, @DingshanDeng, the paper is accepted. Congratulations! I'll shortly generate a post-review checklist of items to work through. The main one will be for you to create an archive of the code to upload to an archiving service like Zenodo or Figshare. I'll also start doing some editorial checks of the paper and either post comments here or open a PR for the finer details. I probably won't get to any of this tonight, though! |
Post-Review Checklist for Editor and AuthorsAdditional Author Tasks After Review is Complete
Editor Tasks Prior to Acceptance
|
@editorialbot set 10.5281/zenodo.14847202 as archive |
Done! archive is now 10.5281/zenodo.14847202 |
@editorialbot set v1.0.0 as version |
Done! version is now v1.0.0 |
@editorialbot check references |
|
@editorialbot generate pdf |
Hi @DingshanDeng, thanks for creating the archive, where everything looks correct. I'll have a closer look at the paper hopefully over the next few days but there are a few things that you can address already.
I think this will already takes us very close to publication, so after these are merged I'll probably open a pull request to address minor editorial points (if there are any). |
Hi @warrickball, thanks for your suggestions and to review this. I updated the draft and I moved the statement of need to the start of the paper. I also updated the journal names in the bib file and now it should look good in the pdf. For the two citations, I could not find their DOI numbers, and it might because the Siess et al. (2000) might be too old to have an assigned DOI number while the Manara et al. (2023) is a book. So I put the url links for the published versions. |
@editorialbot generate pdf |
Thanks @DingshanDeng! I'll have a look over the next few days.
Though we like to have a complete set of DOIs, we are aware that there can be gaps. Both the reasons you mention are quite common and perfectly reasonable. |
I've opened a pull request in the |
Submitting author: @DingshanDeng (Dingshan Deng)
Repository: https://github.com/DingshanDeng/ysoisochrone
Branch with paper.md (empty if default branch):
Version: v1.0.0
Editor: @warrickball
Reviewers: @HeloiseS, @vsquicciarini
Archive: 10.5281/zenodo.14847202
Status
Status badge code:
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
@HeloiseS & @vsquicciarini, 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:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @warrickball 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 @HeloiseS
📝 Checklist for @vsquicciarini
The text was updated successfully, but these errors were encountered: