opendrift
opendrift copied to clipboard
OpenDrift
Add zeta to sdepth in depths.py (part of effort to add zeta to ROMS reader)
This looks very good, including the new unittests. The slight modifications to the existing test seem also very reasonable.
Thus, I might suggest that this should be safe to merge as a clear improvement, before proceeding with other updates, where there are some other challenges as discussed here: https://github.com/OpenDrift/opendrift/issues/1235#issuecomment-1972196208
ok I added in my addition of zeta to the ROMS reader now too which was missing before.
Ref https://github.com/OpenDrift/opendrift/issues/1235, I believe that this PR could be merged.
Some tests are failing, but this is presumably just do to minor changes in depths, and that tests could be updated accordingly. So if you could adjust these tests to pass, I can merge this PR.
Thank you, I should be able to get back to this and other on-going development work on opendrift by mid-next week. In particular I have some changes to allow for the wetting/drying mask that follow up on these changes — should they be in a different PR?
It could be good to merge this first, but you can decide if you prefer to add more before merging.
But CircleCI shows that one of the tests (test_truncate_ocean_model) is failing due to an index error in the ROMS reader:
https://app.circleci.com/pipelines/github/OpenDrift/opendrift/2869/workflows/02ca903c-7b93-4a2e-871c-a9bcc1849066/jobs/13691/parallel-runs/1/steps/1-110
The special config setting o.set_config('drift:truncate_ocean_model_below_m', <depth>) is available for users who want to increase performance by only retrieving the upper few layers of an ocean model, where the lowest layer is extrapolated towards seafloor for any particles below that level:
https://github.com/OpenDrift/opendrift/blob/master/tests/models/test_run_3d.py#L83
So apparently a small fix has to be made in the ROMS reader to account for this possibility.
It turns out that it fails on the first run of test_truncate_ocean_model, before the truncation is applied.
Thus it seems to be a general (indexing) problem in the ROMS reader, not related to the optional truncating.
I can have a closer look at this soon.
I'm digging back into this work today so I'll see what I can figure out too.
I'm digging back into this work today so I'll see what I can figure out too.
Ok. Btw, the depth-module from the Roppy package was copied into Opendrift a long time ago, but it has since evolved also in the Roppy repository: https://github.com/bjornaa/roppy/commits/master/roppy/depth.py
Generally, I think it would be better if OpenDrift could instead use Roppy as an external dependency, instead of just copying this single file. Thus it would be good if your updates with zeta could be incorporated into Roppy official. But @bjornaa should decide whether this is desirable, as any change in the Roppy API might have downstream consequences for others.
Yes I agree that would be a better path. I'd be happy to make changes to roppy directly if @bjornaa would be interested.
Ok how about this: I need to do some clean up on my end to save some subsequent code changes I have and look at that test, but then what if we push the changes in this PR through now. Then, once we hear back from @bjornaa we can presumably subsequently export this capability to roppy instead of having it built into opendrift, but do it as a subsequent step.
Yes, we should anyway document and test the new improvement in OpenDrift, before eventually suggesting the same update to roppy. But before merging into OpenDrift all tests must be passing.
I haven't dug into the docs — is there a place you'd like me to add an update? Or perhaps I could start a blurb about the ROMS reader specifically?
I see that the tests are all failing here but it looks like they fail on build? They are running for me locally anyway.