006 feature sampler near land - #28
Conversation
atb299
left a comment
atb299
left a comment
There was a problem hiding this comment.
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.
|
|
||
| Return | ||
| index: indicies of model in i an j | ||
| index: indicies of model in i and j |
| return profile | ||
|
|
||
|
|
||
| def space_bounds(self, ds, ji, score): |
There was a problem hiding this comment.
Looks okay to me; one question I have is where this filtering should occur in the ObsSampler lifecycle.
One option might be to shift the responsibility of filtering out land-values to the generation of the probability map and your random sample generator? Meaning that, we can never produce a (j, i) pair for a synthetic observation that does not correspond to a wet grid point.
|
|
||
|
|
||
| def find_nearest_time(self, ds, profile, thresh=10): | ||
| def find_nearest_time(self, ds, profile): |
There was a problem hiding this comment.
What was the threshold parameter intended for originally?
There was a problem hiding this comment.
I was thinking a window in time that an obs could be associated with a model point but it didn't make sense.
| assert (model_t.votemper.sel(profile_id=1) | ||
| assert ((model_t.votemper.sel(profile_id=1) | ||
| == synthetic_ds.votemper[5, :, 6, 8]).all() | ||
| & (model_t.sizes['profile_id'] == 1)) |
oj-tooth
left a comment
oj-tooth
left a comment
There was a problem hiding this comment.
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?
|
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. 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. |
3 participants
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.