Skip to content

Migrate to Parcels v4 - #342

Open
j-atkins wants to merge 110 commits into
mainfrom
migrate-v4
Open

Migrate to Parcels v4#342
j-atkins wants to merge 110 commits into
mainfrom
migrate-v4

Conversation

@j-atkins

@j-atkins j-atkins commented May 20, 2026

Copy link
Copy Markdown
Collaborator

This PR migrates to using Parcels v4 for VirtualShip.


  • Update instrument kernels
  • Update fieldset ingestion workflows
  • Update to Parquet output
  • Implement new fieldset/performance handling strategy, i.e. new v4 docs
  • Implement Refactor underway instruments #231
  • Check expedition / workflow
    • including underway instruments
    • Argo float bug [in kernel(s)?], immediately goes out of bounds after first timestep... likely depth convention related
  • Implement initial timestep sampling (i.e. Parcels #2622)
  • Update to work with local data / --from-data
  • Update tests
  • Add new tests for v4 specific logic(?)
  • Update docs / tutorials (e.g. notebooks using .zarr output) [new PR]
    • Plus new requirements for local data ingestion: depth dim has positive: down or positive: up
  • ARGO_FLOATS: Enhance performance + no need to update cycle_age during ascent?

Closes #239, closes #232, closes #231, closes #323, closes #238

@j-atkins
j-atkins changed the base branch from main to update-docs-sensors May 20, 2026 15:02
Base automatically changed from update-docs-sensors to main May 21, 2026 09:08
@j-atkins

j-atkins commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

cc @erikvansebille

This PR is ready for re-review. Please note a couple of things, which have been added more recently, including tinkering around _via_tmp_ds in base.py (e.g. using Zarr stores to try and squeeze out a bit more performance), plus moving to execute instrument simulations with a context manager protocol for a better handling of the tmp directory writing. Also, changing the Argo Float DT (now matching the 5 min output dt) which speeds up the actual Parcels simulation considerably (unless there is a reason to keep it as DT = 10.0?).

Also, there are some changes that have been reviewed already, most notably the updates to the docs, and probably don’t need much time spent on them. Apologies this probably wasn’t the most efficient way of structuring the PRs.

@j-atkins
j-atkins requested review from erikvansebille and removed request for erikvansebille September 4, 2026 09:10

@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.

Impressive effort, @j-atkins! See below some small comments/questions

Comment on lines +242 to +246
fieldsets_list.append(fs)

combined_fieldset = fieldsets_list[0]
for fs in fieldsets_list[1:]:
combined_fieldset = combined_fieldset + fs

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.

Can you not simply keep adding the fieldsets? Instead of first making a list and then adding the entries of that list?

def _sample_salinity(particle, fieldset, time):
particle.salinity = fieldset.S[time, particle.depth, particle.lat, particle.lon]
def _sample_salinity(particles, fieldset):
particles.salinity = fieldset.S[particles.t, particles.z, particles.y, particles.x]

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.

In general, it's better to sample with [particles] since that retains the particles._ei element indices, so makes grid searching faster

Suggested change
particles.salinity = fieldset.S[particles.t, particles.z, particles.y, particles.x]
particles.salinity = fieldset.S[particles]

Also relevant for the other sampling kernels

Comment on lines +106 to +110
particles_lowering.raising = np.where(
particles_lowering.z + particles_lowering.dz < particles_lowering.max_depth,
1,
particles_lowering.raising,
)

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.

Not sure how you feel, but I tend to prefer boolean indexing (like in the Argo float kernel) over np.where(), as I find the boolean indexing easier to read

x=[ctd.spacetime.location.lon for ctd in measurements],
y=[ctd.spacetime.location.lat for ctd in measurements],
z=[ctd.min_depth for ctd in measurements],
t=[np.datetime64(ctd.spacetime.time) for ctd in measurements],

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.

why si this casting to datetime64 needed? Can't it be done without?

try:
value = bathymetry_field.eval(
0, # time
np.float64(0.0), # time

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.

Interesting that you needed this change. Is this a bug in Parcels?

Comment thread pixi.toml

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.

perhaps let @VeckoTheGecko also double-check the pinnings in this pixi.toml?

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.

.csv output Update numpy dependency Add support for Python 3.13/3.14 Integrate Parcels v4 into VirtualShip Refactor underway instruments

2 participants