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

Add "Escaping from the Swirling Vortex" example #389

Conversation

rmsrosa
Copy link
Contributor

@rmsrosa rmsrosa commented Aug 11, 2021

Haven't changed any Core or Test code. Just adding an example. 😃

@Wikunia
Copy link
Member

Wikunia commented Aug 11, 2021

Thanks a lot @rmsrosa
For everyone to see the animation.
animation

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.

That looks awesome. I mean I don't have a clue about most of it but it looks beautiful 😄
Might need to check out the paper 🤔
Thanks for the code and so many comments.

Do you mind running

using JuliaFormatter
format("examples/")

from the root directory as it seems like some indentations are a bit off

end

function ode_arrow(x, y, t, frm_scl, arrow_color)
ode_arrow(x, y, t, frm_scl, [arrow_color, arrow_color])
Copy link
Member

Choose a reason for hiding this comment

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

I think the [, ] shouldn't be there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that is right. That is reminiscent of a failed attempt to add a blue arrow for the trajectory itself. I better just remove this dispatch for this example since it is not used.

As for the JuliaFormatter.jl, I was hoping to get away without it, but I guess I was caught 😆 . I never used that and I understood I need to install in my global env, right? I will give it a try.

Copy link
Member

Choose a reason for hiding this comment

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

No worries about the formatter. I can also run this 😉

Yes make sure to update the docstring as well then for the ode_arrow function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I just run JuliaFormatter.jl. It was actually a good thing to do. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Great. You only ran it on the examples, right? Maybe try running it on all with format(".") as we have some weirdness in src/latex... which I can't replicate on my machine 😄

@rmsrosa
Copy link
Contributor Author

rmsrosa commented Aug 11, 2021

Hmm, how come the format-check failed? I just ran format("examples/") and didn't change anything on the code after that.

Oh, it is complaining about src/latexsvgfile.jl, not about the example I added.

@Wikunia
Copy link
Member

Wikunia commented Aug 11, 2021

Yeah formatting is a bit weird sometimes especially with that file. It says it's fine on my machine but the CI doesn't like it. Wondering what your machine says 😄

@rmsrosa
Copy link
Contributor Author

rmsrosa commented Aug 11, 2021

Ok, format(".") did change src/latexsvgfile.jl but the formatting seems hopeless... The lines are really long.

@Wikunia
Copy link
Member

Wikunia commented Aug 11, 2021

Yeah unfortunately it doesn't seem like one can ignore files... Anyhow that's not your problem 😄 We'll figure something out.

@codecov
Copy link

codecov bot commented Aug 11, 2021

Codecov Report

Merging #389 (f87e7f5) into master (9d6b289) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #389   +/-   ##
=======================================
  Coverage   96.24%   96.24%           
=======================================
  Files          35       35           
  Lines        1519     1519           
=======================================
  Hits         1462     1462           
  Misses         57       57           

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 9d6b289...f87e7f5. Read the comment docs.

@TheCedarPrince
Copy link
Member

I'd say we could probably go ahead and merge this as is. Those particular latex files always give us issues anyhow. :/

@Wikunia
Copy link
Member

Wikunia commented Aug 11, 2021

format is fixed in master now.
Merged 😉

@Wikunia Wikunia merged commit 332f878 into JuliaAnimators:master Aug 11, 2021
@Wikunia Wikunia mentioned this pull request Aug 11, 2021
@Sov-trotter
Copy link
Member

I think this deserves an entry in the Summer of Math Exposition. Also it would be good place to show off Javis.

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.

4 participants