Skip to content

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.

4 participants