-
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
Implement the new particle spray model by Chen+24 #670
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #670 +/- ##
=======================================
Coverage 99.91% 99.91%
=======================================
Files 200 200
Lines 29269 29322 +53
Branches 564 564
=======================================
+ Hits 29243 29296 +53
Misses 26 26 ☔ View full report in Codecov by Sentry. |
1d8d336
to
5867d7d
Compare
Thanks for starting the PR. You might want to install and use precommit as discussed in the development installation docs that I linked to in my email. That would avoid all the tweaks by precommit. |
Thank you! This is very useful. |
Hi @jobovy, This PR have passed most checks! I also added some new tests, documentation, and updated Let me know if more is needed! -Bill |
Thanks! I’ll review this next week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ybillchen --I did a first review mainly focusing on the code in streamspraydf
and the tests, but not yet the documentation. Overall it looks great and great job on the tests! I have a bunch of comments that should be addressed before this can be merged. I might have more comments later, these are just some major ones to get started. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few more small comments on the latest changes.
Sorry for the delay in finishing this review, I'm hoping to get to this later this week. |
…tor's potential for stream integration
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Move progenitor's potential to __init__() Simplify syntax
0092d64
to
62f2208
Compare
Sorry for the delay, but I was finally able to do a final review of the documentation updates and after a few small tweaks was able to merge this. Thanks again for your contribution @ybillchen! |
Implement the new particle spray model by Chen+24 and include progenitor's potential for stream integration