Skip to content

006 feature sampler near land - #28

Merged
oj-tooth merged 5 commits into
mainfrom
006-feature-sampler-near-land
Aug 17, 2026
Merged

006 feature sampler near land#28
oj-tooth merged 5 commits into
mainfrom
006-feature-sampler-near-land

Conversation

@b-barton

Copy link
Copy Markdown
Collaborator

This fixes #6 . The code now account for profiles that are sampled on the model grid and fall on land. These profiles are rejected and not returned in the dataset of model profiles.

@b-barton
b-barton requested review from atb299 and oj-tooth July 24, 2026 12:45
@b-barton b-barton self-assigned this Jul 24, 2026
@b-barton b-barton added the mod:sampler Relates to sampling modules. label Jul 24, 2026
@b-barton b-barton linked an issue Jul 24, 2026 that may be closed by this pull request

@atb299 atb299 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The code all reads OK to me, so I think it is OK to be merged. I will leave that step for Ollie, since this will be merged with main and I am still not entirely familiar with the repo.

Comment thread OceanOSSE/sampling/sampler_nearest_neighbour.py Outdated
Comment thread OceanOSSE/sampling/sampler_nearest_neighbour.py
Comment thread OceanOSSE/sampling/sampler_nearest_neighbour.py
Comment thread tests/unit/test_sampler.py

@oj-tooth oj-tooth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Happy with this PR, but wanted to pose my question on where the land-sea filtering implementation should happen before we merge into main?

It may be possible to move this filtering earlier in the ObsSampler lifecycle, ensuring we do not generate any (j, i) model coordinates for synthetic profiles that are located on land and then filtering during extraction would not be needed.

Would be good to get @b-barton and @atb299 thoughts on this?

@b-barton

b-barton commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

I think the nearest neighbour sampler sits in parallel to the RandomSampler. In config you will pick which route your going. I thought either you pass it a dataset of profiles you want to replicate in model space or you want a random set from the probability distribution?

For the wet cell filtering, I think it need to happen in both places.
Off issue but the probability map doesn't necessarily correspond to the model grid you want to apply it to so they might have different land masks. The probability map gets interpolated to the model grid.

If there is a better place for it, it could get moved later. I'm not sure where that would be though because before ObsSampler is DataLoading.

@oj-tooth oj-tooth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me!

Let's get this into main and work on testing the complete OceanOSSE pipeline.

@oj-tooth
oj-tooth merged commit e61efc5 into main Aug 17, 2026
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mod:sampler Relates to sampling modules.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[feature] Sampler near land

3 participants