Migrate to Parcels v4 - #342
Conversation
…ument column entries are consistent
for more information, see https://pre-commit.ci
f474279 to
0fe1340
Compare
…able tmp ds writing without heavy RAM spikes
…ting and "simulating" print
|
This PR is ready for re-review. Please note a couple of things, which have been added more recently, including tinkering around 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. |
erikvansebille
left a comment
erikvansebille
left a comment
There was a problem hiding this comment.
Impressive effort, @j-atkins! See below some small comments/questions
| fieldsets_list.append(fs) | ||
|
|
||
| combined_fieldset = fieldsets_list[0] | ||
| for fs in fieldsets_list[1:]: | ||
| combined_fieldset = combined_fieldset + fs |
There was a problem hiding this comment.
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] |
There was a problem hiding this comment.
In general, it's better to sample with [particles] since that retains the particles._ei element indices, so makes grid searching faster
| particles.salinity = fieldset.S[particles.t, particles.z, particles.y, particles.x] | |
| particles.salinity = fieldset.S[particles] |
Also relevant for the other sampling kernels
| particles_lowering.raising = np.where( | ||
| particles_lowering.z + particles_lowering.dz < particles_lowering.max_depth, | ||
| 1, | ||
| particles_lowering.raising, | ||
| ) |
There was a problem hiding this comment.
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], |
There was a problem hiding this comment.
why si this casting to datetime64 needed? Can't it be done without?
There was a problem hiding this comment.
I get an error when using the raw datetime.datetime objects:
RuntimeError: An unexpected error occurred while simulating measurements: particle t must be a datetime, timedelta, or date object.
And I find the same error outside of VirtualShip. Given the error messaging suggests datetime objects should be supported, I presume this is a bug. I'll can make an issue on the Parcels tracker!
| try: | ||
| value = bathymetry_field.eval( | ||
| 0, # time | ||
| np.float64(0.0), # time |
There was a problem hiding this comment.
Interesting that you needed this change. Is this a bug in Parcels?
There was a problem hiding this comment.
I think this is leftover from a workaround for a bug which may have existed previously (?) but is now seemingly resolved in Parcels. Therefore, will be removed.
There was a problem hiding this comment.
perhaps let @VeckoTheGecko also double-check the pinnings in this pixi.toml?
…ate tests with more tolerance for interpolation discrepancies
|
Thanks for the review, @erikvansebille! Comments now addressed, including moving to using boolean indexing across all (non-underway) instrument kernels. @VeckoTheGecko I have updated/unpinned pixi in the |
2 participants
This PR migrates to using Parcels v4 for VirtualShip.
Argo float bug [in kernel(s)?], immediately goes out of bounds after first timestep... likely depth convention related--from-data.zarroutput) [new PR]positive: downorpositive: upARGO_FLOATS: Enhance performance + no need to updatecycle_ageduring ascent?Closes #239, closes #232, closes #231, closes #323, closes #238