Skip to content

Compatability updates for parcelsv4 - #50

Open
wyatt-fluidnumerics wants to merge 7 commits into
mainfrom
parcels-benchamrks-compatability-updates-for-parcelsv4
Open

Compatability updates for parcelsv4#50
wyatt-fluidnumerics wants to merge 7 commits into
mainfrom
parcels-benchamrks-compatability-updates-for-parcelsv4

Conversation

@wyatt-fluidnumerics

Copy link
Copy Markdown

The goal of the PR is to update the current benchmarking suite to work with the most up to date version of parcels v4. This PR achieves this goal in the following ways:

  • Pixi and pixi-build-python versions are updated to match the newest parcels versions.
  • A new commit for ./Parcels is pinned in the submodule
  • All benchmarking simulations are given the _delete_error_particle kernel to avoid benchmarks crashing when a particle goes out of bounds.
  • .to_windowed_array() is called on benchmarking field sets.
  • Changes to variable naming conventions in parcels v4 are updated in benchmarking simulations (i.e. lat/lon are no longer supported when creating particlesets).
  • Revise the choice of velocity interpolator in the moi benchmark.

@erikvansebille erikvansebille left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good; just one small change suggested below

Comment on lines 65 to 68
if interpolator == "XLinear":
fieldset.U.interp_method = XLinear
fieldset.V.interp_method = XLinear
fieldset.UV.interp_method = XLinear_Velocity()
else:
raise ValueError(f"Unknown interpolator: {interpolator}")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is old code from when we didn't have a working CGrid_Velocity interpolator yet. I would now simply use the default interpolator that comes with the FieldSet

@fluidnumericsJoe fluidnumericsJoe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good. Just one question - with the change in results.json, I'm assuming this is reflective of you running the benchmarks with the changes on this branch, yeah?

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.

3 participants