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

Example - Gravities of the Solar System #432

Merged
merged 12 commits into from Oct 25, 2021

Conversation

MisterBiggs
Copy link
Contributor

PR Checklist

If you are contributing to Javis.jl, please make sure you are able to check off each item on this list:

  • Did I update CHANGELOG.md with whatever changes/features I added with this PR?
  • Did I make sure to only change the part of the file where I introduced a new change/feature?
  • Did I cover all corner cases to be close to 100% test coverage (if applicable)?
  • Did I properly add Javis dependencies to the Project.toml + set an upper bound of the dependency (if applicable)?
  • Did I properly add test dependencies to the test directory (if applicable)?
  • Did I check relevant tutorials that may be affected by changes in this PR?
  • Did I clearly articulate why this PR was made the way it was and how it was made?

Link to relevant issue(s)
Closes #360

How did you address these issues with this PR? What methods did you use?

Not sure if you would like to bump the version in the CHANGELOG.md just for an example.

Otherwise, the code in this PR is just for a new example. The example has been added to the repos /examples folder as well as the Examples in the docs. Below is an example of the finished animation!

test

Copy link
Member

@Wikunia Wikunia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The animation looks awesome! Thanks for creating the PR. I think we should try to use act! to update the positions of each planet though instead of creating new objects every time.
You can find some examples and explanations regarding that in my latest blog posts: https://opensourc.es
Let us know when you have any questions.

examples/gravities.jl Outdated Show resolved Hide resolved
examples/gravities.jl Outdated Show resolved Hide resolved
Copy link
Member

@Wikunia Wikunia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether this was ready for review again ;) Please ping me next time you want another review. Thanks for rewriting it with actions. Let me know if you need any help or whether we can make some stuff more clear in tutorials and such.

examples/gravities.jl Outdated Show resolved Hide resolved
examples/gravities.jl Outdated Show resolved Hide resolved
@MisterBiggs
Copy link
Contributor Author

My bad I forgot the pr wasn't a draft, definitely still some work to be done before it's ready. I've been struggling to do this in an example-worthy way without something to properly represent forces like #423. I also think this would be a great example to put out with forces to showcase how they work. I'm curious about opinions on if I should wait for #423 to be implemented before finishing this animation. @Wikunia

I feel pretty confident that I could implement #423 I just want to look into the internals of Javis a little more before I commit to doing it.

@MisterBiggs MisterBiggs marked this pull request as draft October 14, 2021 18:02
@Wikunia
Copy link
Member

Wikunia commented Oct 14, 2021

I think in the long run we definitely want to have #423 in this example. However it might make sense to try it out without it first and learn the best approach of how to make it internal later on.

@TheCedarPrince
Copy link
Member

As an additional comment, I would say that #423 would be best to go into another Javis package called something like JavisPhysics. Now since we have the JuliaAnimators organization, we could go ahead and create that repository to handle prototyping of these ideas that are more domain specific.

@MisterBiggs
Copy link
Contributor Author

As an additional comment, I would say that #423 would be best to go into another Javis package called something like JavisPhysics. Now since we have the JuliaAnimators organization, we could go ahead and create that repository to handle prototyping of these ideas that are more domain specific.

I was thinking of suggesting making it its own package as well. Being an engineer might make me think too big for the intended use case in Javis, but I think either keep the scope of physics/forces to how it's currently outlined in #423, and if users want to do something more complicated, they can do a for loop like my original PR. Otherwise, feature creep related to physics could get pretty excessive and would warrant its own package.

@Sov-trotter
Copy link
Member

Hey people. This PR Looks really solid!

However it might make sense to try it out without it first and learn the best approach of how to make it internal later on.

I can't agree more @Wikunia. Before having some rigid math/physics/any computational stuff it would be better to have multiple examples before hand. We could divide and exand the examples sections though(based on domain). Also I would love to see how we can possibly integrate other physics based/domain specific packages from the julia package ecosystem since those will be much more performant and also save us time.

examples/gravities.jl Outdated Show resolved Hide resolved
@MisterBiggs MisterBiggs marked this pull request as ready for review October 24, 2021 01:38
@codecov-commenter
Copy link

codecov-commenter commented Oct 25, 2021

Codecov Report

Merging #432 (b170e14) into master (5781699) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #432      +/-   ##
==========================================
- Coverage   96.19%   96.18%   -0.02%     
==========================================
  Files          36       36              
  Lines        1604     1598       -6     
==========================================
- Hits         1543     1537       -6     
  Misses         61       61              
Impacted Files Coverage Δ
src/Javis.jl 95.94% <0.00%> (-0.09%) ⬇️
src/shorthands/JLine.jl 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5781699...b170e14. Read the comment docs.

Copy link
Member

@Wikunia Wikunia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome thanks for this stunning work.

@Wikunia Wikunia merged commit 7797fb1 into JuliaAnimators:master Oct 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Idea for a Javis example
5 participants