-
-
Notifications
You must be signed in to change notification settings - Fork 104
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
Submission: exoplanets #309
Comments
Editor checks:
Editor commentsThis looks like a really interesting package, thanks for submitting! We run a few standard checks on all new packages, using From From From Here are some small fixes to consider based on those checks:
For the next step, I'll assign reviewers for the submission. Please let me know if you have any questions during the process! |
Awesome! I've added I have shortened a few of the > 80 lines. A few remain but they are mostly due to a really long API URL. I would prefer to retain that URL instead of |
@tyluRp : Thanks so much for those updates. I don't think we have hard-and-fast rules for anything in the packages, just guidance. If you have thought about good practice guidelines and have a case where it makes sense to you to make an exception, that seems reasonable. We now have one reviewer assigned (@h21k) and I am continue to look for the second reviewer. |
We now have our second reviewer: @nmonhait. I'm assigning a due date for reviews of August 5. |
Yay! Excited for feedback, thank you!! |
Hi @tyluRp , I will need to submit later this week. Sorry for the delay! |
@nmonhait No problem at all! Take your time 👍 |
Hi all,
Apologies, I would also need some time to be able to submit (approx around
the 20th of August).
Very best wishes,
Frank
…On Wed, 7 Aug 2019 at 18:17, Tyler Littlefield ***@***.***> wrote:
@nmonhait <https://github.com/nmonhait> No problem at all! Take your time
👍
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#309?email_source=notifications&email_token=AGH7LOCQNRY57RKDR6CDGKLQDNX2NA5CNFSM4HPU56IKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD32EJLQ#issuecomment-519324846>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AGH7LOD6FZIC3D46HEKNX33QDNX2NANCNFSM4HPU56IA>
.
--
Dr Frank Soboczenski
Personal: https://h21k.github.io
NASA Globe: https://www.globe.gov/web/frank.soboczenski/home
NASA FDL: http://frontierdevelopmentlab.org/
Twitter: h21k
Latest work on gravitational wave ML models:
https://github.com/h21k/GravitationalWavesML
|
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:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 5
Review CommentsHi @tyluRp, great work on this package! My comments are below: README
General devtools::check() 0 errors ✔ | 0 warnings ✔ | 0 notes ✔ devtools::test() OK: 9 goodpractice::gp() ✖ avoid long code lines, it is bad for readability. Also, many people prefer editor windows that are about 80 characters wide.
To summarize I think you did a great job. My main suggestion is to add more detail aimed at the 'first-touchpoint' user. Thorough documentation and vignettes will allow newcomers to utilize the functionality of this package. |
@nmonhait Thanks a bunch for all the feedback! I will start working on the things you have mentioned. I am curious about the LaTeX errors. If possible, could you elaborate? I do recall adding TODO:
Let me know if I missed anything. Additionally, I recently made a somewhat large change to the package. I've decided to use Thanks again for the feedback 👍 |
Thank you so much for your review, @nmonhait! @h21k, do you have an update on when you would be able to add your review? @tyluRp : Regarding your Windows question, I'm using a Mac, so I can't check. Perhaps @nmonhait or @h21k is using Windows and could check. Also, have you tried out the package through http://win-builder.r-project.org/? I'm not sure if that's going to provide more details for you than the Appveyor output, but it might be worth trying if you haven't yet. If you're using the |
@geanders Thanks for the advice. I've just tried this out on a windows machine and I am getting the same error: Error in curl::curl_fetch_memory(url, handle = handle) :
Failure when receiving data from the peer I will use the UPDATE: Wanted to provide an update for any Windows users. I filed an issue for the I've decided to add two utility functions to the package for handling requests differently depending on the operating system (windows, or anything else), you can see the additions here ropensci/exoplanets@6b93fb2. This has fixed the appveyor build. |
Hi @nmonhait, thanks again for your feedback. I've added a vignette with a walk through on how to use the package and access data from the archive. I've also added additional helper functions for the time series datasets, the intro vignette covers these new functions as well. Code of conduct and contributing guidelines have been added. When you have a moment, please let me know your thoughts. |
Hi all,
apologies for the delay.
I should have mine ready very soon.
Best wishes,
Frank
…On Sun, 3 Nov 2019 at 04:01, Tyler Littlefield ***@***.***> wrote:
Hi @nmonhait <https://github.com/nmonhait>, thanks again for your
feedback. I've added a vignette with a walk through on how to use the
package and access data from the archive. I've also added additional helper
functions for the time series datasets, the intro vignette covers these new
functions as well.
Code of conduct and contributing guidelines have been added. When you have
a moment, please let me know your thoughts.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#309?email_source=notifications&email_token=AGH7LOEYMDEZN7QNK26VURDQRZEIHA5CNFSM4HPU56IKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC5KNCA#issuecomment-549103240>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGH7LOEQH5LK2IUO7EY62DLQRZEIHANCNFSM4HPU56IA>
.
|
Thank you for the update, @h21k! Looking forward to your review! |
@h21k : I just wanted to check in and see if you will be able to have your review in soon? Thanks again so much for reviewing this package! |
@h21k: I wanted to check and see if you will still be able to review this package? |
In the interest of reducing load on reviewers and editors as we manage the COVID-19 crisis, rOpenSci is temporarily pausing new submissions for software peer review for 30 days (and possibly longer). Please check back here again after 17 April for updates. In this period new submissions will not be handled, nor new reviewers assigned. Reviews and responses to reviews will be handled on a 'best effort' basis, but no follow-up reminders will be sent. Other rOpenSci community activities continue. We express our continued great appreciation for the work of our authors and reviewers. Stay healthy and take care of one other. The rOpenSci Editorial Board |
👋 @tyluRp I'll handle this submission from now on. @geanders will be the second reviewer. Thanks for your patience! |
@maelle Sweet, appreciate it! I'll keep an eye out. |
@ropensci-review-bot add @geanders to reviewers |
@geanders added to the reviewers list. Review due date is 2021-03-17. Thanks @geanders for accepting to review! Please refer to our reviewer guide. |
@geanders friendly reminder about your review 🙂 |
@ropensci-review-bot add @maelle to reviewers |
@maelle added to the reviewers list. Review due date is 2021-05-22. Thanks @maelle for accepting to review! Please refer to our reviewer guide. |
@ropensci-review-bot remove @geanders from reviewers |
@geanders removed from the reviewers list! |
@tyluRp about to start reviewing, sorry for the delay and thanks for your patience! |
Thanks again for your submission and patience @tyluRp! 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:
Functionality
Estimated hours spent reviewing: 1
Review CommentsThis is a nice package for people who'll need to use this data in R! 💫 CRAN submissionI see traces of a CRAN submission, but the packages isn't on CRAN yet, what happened? In any case, now it'd make sense to wait until after the review changes to re-submit. Also note that there is no obligation to submit the package to CRAN. Documentation
Contributing informationThere is no CONTRIBUTING.md at the moment. When time comes to transfer your repository to the ropensci organization, we'll ask you remove the Code of Conduct file as rOpenSci Code of Conduct will apply to your package. Code
TestingTests currently fail, but I also find them a bit light at the moment.
Continuous integration
Thanks again for your submission & work on this neat&useful package! 🪐 |
Thanks for the review!
The 60 day scheduled check sounds really nice, especially for a package that depends on an API. I'll review this feedback over the weekend and start updating things. Thanks! |
@maelle Okay! So I've gone ahead an basically rewrote the entire package due to API changes. AFAIK, the function used to get column information broke last Friday so basically everything broke as you mentioned. This rewrite uses the new "TAP" service offered by the exoplanet archive. I've decided to support the new interface for now as it looks like tables from the old interface are planned to be migrated to "TAP" in the future. If things settle down, I may decide to support the old API. Summary of changes:
Pending items:
Anyway, this should be good enough to actually see the package in action. I will post an update once I'm happy with the tests. Please let me know if you have any questions. |
Alright, I think I'm done, I've added:
Also wanted to say thanks a bunch, this feedback has taught me quite a bit, specifically:
Lastly, going back to the |
Thanks @tyluRp for all your work on this! 🚀 ✨ My answers
Thanks again! |
One last question actually, why keep the random aspect of the tests and not add tests corresponding to the same use cases as the examples in the manual? |
Please let me know if theres anything else I missed! |
By the way, the
But |
Some comments (probably last comments). Thanks for your work!
|
Please let me know if there is anything else :) |
@ropensci-review-bot approve |
Approved! Thanks @tyluRp for submitting and @nmonhait, @maelle for your reviews! 😁 To-dos:
Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them Welcome aboard! We'd love to host a post about your package - either a short introduction to it with an example for a technical audience or a longer post with some narrative about its development or something you learned, and an example of its use for a broader readership. If you are interested, consult the blog guide, and tag @stefaniebutland in your reply. She will get in touch about timing and can answer any questions. We've put together an online book with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved, the corresponding repo is here. Last but not least, you can volunteer as a reviewer via filling a short form. |
Thanks @tyluRp! Happy to answer any question you might have! |
@maelle I think I got most of it except:
|
@tyluRp
|
Submitting Author: Tyler Littlefield (@tyluRp)
Due date for @nmonhait: 2020-08-05Repository: https://github.com/tyluRp/exoplanets
Version submitted: 0.0.0.9000
Editor: @maelle
Reviewers: @nmonhait, @maelle
Due date for @maelle: 2021-05-22
Archive: TBD
Version accepted: TBD
Scope
Please indicate which category or categories from our package fit policies this package falls under: (Please check an appropriate box below. If you are unsure, we suggest you make a pre-submission inquiry.):
Explain how and why the package falls under these categories (briefly, 1-2 sentences):
Who is the target audience and what are scientific applications of this package?
Are there other R packages that accomplish the same thing? If so, how does yours differ or meet our criteria for best-in-category?
If you made a pre-submission enquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted.
Technical checks
Confirm each of the following by checking the box. This package:
Publication options
JOSS Options
paper.md
matching JOSS's requirements with a high-level description in the package root or ininst/
.MEE Options
Code of conduct
The text was updated successfully, but these errors were encountered: