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

Adding new tutorial on the impact of grid and velocity interpolation scheme on trajectories #1769

Merged
merged 9 commits into from
Dec 5, 2024

Conversation

erikvansebille
Copy link
Member

This new tutorial was developed with @sruehs and @michaeldenes during discussions on the relation between grids and interpolation schemes.

Copy link
Contributor

@VeckoTheGecko VeckoTheGecko left a comment

Choose a reason for hiding this comment

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

I have made some refactorings. I have pushed them, but here is a rich diff

suggested_diff.html.zip

Changes:

  • Introduce utility class to consolidate experiment handling (getting interpolation method, advection kernel, file name, plot title)
  • patch fstring to be 3.10 compatible
  • rename matplotlib variables to align with convention
  • update plotting code to manually handle legend

Ignore the sharex= in the plt.subplots code in the diff (its not needed, and not in the patch).

These changes don't affect the simulations run. I ran the notebook before and after, and there is no visual changes in the plots.

@erikvansebille
Copy link
Member Author

Thanks for this clean-up, @VeckoTheGecko. Defining a class is perhaps a bit overkill for a tutorial (we don't do it in other tutorials) but on the other hand can also inspire Parcels users (including me) to work more with classes themselves

Note that there was a small change in your last figure, where the red dots ('Cgrid and analytical advection' were different in your rendering. If I rerun your code, I get the original values for these experiments. Not sure why you get (slightly) different results for analytical advection; perhaps the version of scipy?? Anyways, since we're working on a newer, more robust implementation of AnalyticalAdvection anyways (see #1612), I don't worry too much for now

@erikvansebille
Copy link
Member Author

@michaeldenes or @sruehs, do you still have comments/feedback on this tutorial before I merge into master? We can of course still update/amend it after merging too

@VeckoTheGecko
Copy link
Contributor

Note that there was a small change in your last figure,

Yes, I noticed that as well. I agree that its likely dependencies

Defining a class is perhaps a bit overkill for a tutorial

Yes, that's fair. If it hinders understanding I'm happy to revert (I'm not too committed to the class approach - I just find it more readable but I'm also not the average Parcels user, so good to know what others think :) )

Copy link
Member

@michaeldenes michaeldenes left a comment

Choose a reason for hiding this comment

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

I've added some specific comments (via GitHub) to the notebook.

@michaeldenes
Copy link
Member

Sorry for the delay in review, didn't see this until today!

Copy link
Member

@michaeldenes michaeldenes left a comment

Choose a reason for hiding this comment

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

Looks good to me now!

@erikvansebille erikvansebille merged commit 29ecf0c into master Dec 5, 2024
16 checks passed
@erikvansebille erikvansebille deleted the tutorial_peninsula_AvsCgrid branch December 5, 2024 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants