Add XBT instrument simulation based on CTD

iuryt commented 2 weeks ago

Summary

This PR introduces a new instrument simulation, xbt.py, located in the instruments/ directory. The xbt.py file is modeled on ctd.py and is designed to simulate data collection for Expendable Bathythermographs (XBTs), which capture water temperature at varying depths.

Changes

New Instrument: Added xbt.py to instruments/ directory for XBT data simulation.
Test: Added test_xbt.py based on test_ctd.py
Init Update: Modified instruments/__init__.py to include xbt.py in the module exports.

Notes

Status: Ready to review.

Next Steps

Update documentation to reflect the addition of the XBT instrument.

iuryt commented 2 weeks ago

It passed the tests (including the one I created for XBT), but I have not tried or implemented it in a tutorial. This PR is ready to review. My only question is that XBTs have fixed pre-determined max depth. Should we leave for the users to set it up just like CTDs?

I hope I made your day with this PR, @ammedd ! haha

iuryt commented 2 weeks ago

It seems like this is failing the test because it is reaching almost at the maximum depth, but not really there (e.g. obs_value=7.965900421142578 exp_value=8).

Do you have any idea what it could be?

FAILED tests/instruments/test_xbt.py::test_simulate_xbts - AssertionError: Observation incorrect xbt_i=0 loc='maxdepth' var='temperature' obs_value=7.965900421142578 exp_value=8.

https://github.com/OceanParcels/virtualship/actions/runs/11781564925/job/32816812061#step:5:1886

Edit: I think I know. Maybe this is because the fall speed is a float number and basically we reach the condition (particle.depth + particle_ddepth < particle.max_depth) to delete the particle when we are very close to the bottom, i.e. the particle would cross max_depth if we advance one more time step.

If you agree, we can either change the dt at the last timestep so that we reach the max_depth or we change the way we check the test. Let me know what you think.

erikvansebille commented 1 week ago

I think I know. Maybe this is because the fall speed is a float number and basically we reach the condition (particle.depth + particle_ddepth < particle.max_depth) to delete the particle when we are very close to the bottom, i.e. the particle would cross max_depth if we advance one more time step.

Yes I agree that would be the likely culprit

If you agree, we can either change the dt at the last timestep so that we reach the max_depth or we change the way we check the test. Let me know what you think.

Hmm, changing dt would just make the code slower. We could instead either accept the current situation (the xbt line breaks very close to max_depth), or write another statement so that the last depth is exactly max_depth. Something like (not tested)

# Delete particle if depth is exactly max_depth
if particle.depth + particle_ddepth == particle.max_depth:
    particle.delete()

# Set particle depth to max depth if it's too deep
if particle.depth + particle_ddepth < particle.max_depth:
    particle_ddepth = particle.max_depth - particle.depth

Since the second if-statement will be reached before the first one, the value at max_depth will be written? Can you try?

iuryt commented 1 week ago

It was never reaching the first condition, I changed it to particle.depth == particle.max_depth

I ran the pytests and it worked locally. What do you think?

erikvansebille commented 1 week ago

Yep that sounds correct.

erikvansebille commented 1 week ago

CI breaking is because of issue with Parcels v3.1.0; will be fixed by https://github.com/OceanParcels/virtualship/pull/82.

Shall I merge this PR, @iuryt?

iuryt commented 1 week ago

@erikvansebille I think we can merge. Based on what @ammedd is doing, I can work on examples and documentation later. Thanks! I am very happy to collaborate with this project!

OceanParcels / virtualship