Cropping overlap control using cone distance #1554
Conversation
config/default_config.yml
Outdated
| hl_mask: 3 # Can be different from teacher | ||
| rate: 0.4 # Student cone: 40% of sphere (~72° radius) | ||
| method: "geodesic_disk" # Must be geodesic_disk | ||
| center_distance_degrees: 180 # REQUIRED: Student center is 45° from teacher |
There was a problem hiding this comment.
This comment is a little unclear, the center_distance_degrees is required, but the center_distance_degrees can be any chosen angle right?
There was a problem hiding this comment.
Perhaps you could also include a range as below
There was a problem hiding this comment.
Indeed, the comment is confusing as I had to set center_distance_degrees to 45 before submitting the PR ( I was trying different values to see if they work)
Any angle you choose should work as demonstrated in the example below.
| # number of healpix cells | ||
| self.healpix_level_data = cf.healpix_level | ||
| self.healpix_num_cells = 12 * (4**cf.healpix_level) | ||
| self._hp_cache = {} |
There was a problem hiding this comment.
Good, I think this is helpful
There was a problem hiding this comment.
True, it helped speeding up the computation (we do not have to repeat hp calculation every single time)
src/weathergen/datasets/masking.py
Outdated
| instance default if None. | ||
| masking_strategy_config : dict | None | ||
| Optional override of strategy config (e.g., {'hl_mask': 3}). | ||
| target_metadata : dict | None |
There was a problem hiding this comment.
Perhaps state here that this is required when we do overlap with the cones? Otherwise it is unused right?
There was a problem hiding this comment.
Agree, this is only used when doing cone overlap. I am not sure if there is a more elegant way to do it without adding target_metadata
| "relationship 'cone_distance' requires 'center_cell' in target_metadata" | ||
| ) | ||
| # Get teacher's hl_mask level to properly convert coordinates | ||
| teacher_hl_mask = target_metadata.get("hl_mask", masking_strategy_config.get("hl_mask", 0)) |
There was a problem hiding this comment.
Should this default to 0? Perhaps should check that hl_mask is specified with an assert like the above lines? Do we default to 0 in other parts of the code?
wael-mika
force-pushed
the
wm/dev/cropping_overlap
branch
from
4fb73fd to
613f606
Compare
wael-mika
force-pushed
the
wm/dev/cropping_overlap
branch
from
0882da9 to
796b919
Compare
…t relationship with a small code improvement
2 participants
Description
This PR implements cone distance for spatial sampling. The core logic uses angular distance (in degrees/radii) to determine the spacing between the centers of the Teacher and Student cones/crops.
Key Features:
Geodesic Support: Currently optimized to work specifically with geodesic_disk.
Multi-Level Sampling: Allows Student and Teacher crops to be sampled from different hl_level masks.
Performance Optimizations: I’ve implemented caching for the Healpix object initialization to improve efficiency.
Geometric Logic: Uses spherical trigonometry to calculate destination points given a starting (lon, lat), distance, and azimuth (bearing).
Configuration:
Usage Example: Refer to default_config for the necessary parameters and an explanation of how center_distance_degrees is used for overlap control.
Documentation: I have included long/detailed docstrings for all new functions to make the review process easier.
Issue Number
Closes #1515
Is this PR a draft? Mark it as draft.
Checklist before asking for review
./scripts/actions.sh lint./scripts/actions.sh unit-test./scripts/actions.sh integration-testlaunch-slurm.py --time 60