Skip to content

Conversation

@iuryt
Copy link
Collaborator

@iuryt iuryt commented Nov 5, 2024

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 iuryt marked this pull request as ready for review November 5, 2024 17:21
@iuryt
Copy link
Collaborator Author

iuryt commented Nov 5, 2024

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

Copy link
Member

@erikvansebille erikvansebille left a comment

Choose a reason for hiding this comment

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

Nice addition, @iuryt! Thanks for the contribution. Can you lok at these review comments I provided?

@iuryt
Copy link
Collaborator Author

iuryt commented Nov 11, 2024

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
Copy link
Member

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
Copy link
Collaborator Author

iuryt commented Nov 12, 2024

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
Copy link
Member

Yep that sounds correct.

@erikvansebille
Copy link
Member

CI breaking is because of issue with Parcels v3.1.0; will be fixed by #82.

Shall I merge this PR, @iuryt?

@iuryt
Copy link
Collaborator Author

iuryt commented Nov 12, 2024

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

@erikvansebille erikvansebille merged commit ea3789c into Parcels-code:main Nov 13, 2024
8 checks passed
@ammedd ammedd mentioned this pull request Feb 12, 2025
2 tasks
@iuryt iuryt deleted the xbt_its branch September 8, 2025 17:26
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.

2 participants