Wm/1270/cropping #1447
Wm/1270/cropping #1447
Conversation
…les and overlap + fixing the num_sample bug
clessig
left a comment
clessig
left a comment
There was a problem hiding this comment.
Thanks. I left some comments. Did you generate plots of the different masks, as discussed. The final correctness is really hard to judge from reading the code or even running it.
| mask = np.zeros(num_cells, dtype=bool) | ||
| mask[child_indices] = True | ||
|
|
||
| elif strategy == "cropping_healpix": |
There was a problem hiding this comment.
Can we put this into a separate function
| # iterate over all target samples | ||
| target_masks: list[np.typing.NDArray] = [] | ||
| target_metadata: list[SampleMetaData] = [] | ||
| target_config_mapping = [] # Track which config each target mask came from |
There was a problem hiding this comment.
Why do we need the target_config_mapping. The target_metadata should contain all relevant information, not?
| overlap_set = set(overlap_with) | ||
|
|
||
| # Use intelligent center selection based on overlap target | ||
| if overlap_ratio > 0.7: |
There was a problem hiding this comment.
Where does the 0.7 and the 0.3 below come from?
| crop2 = _select_spatially_contiguous_cells(0, 9, method="geodesic_disk", | ||
| overlap_with=crop1, overlap_ratio=0.3) | ||
| """ | ||
| import warnings |
There was a problem hiding this comment.
Please put all imports at the beginning of the file
| """ | ||
| self.rng = rng | ||
|
|
||
| def _select_spatially_contiguous_cells( |
There was a problem hiding this comment.
We should break this function up into smaller functions that cover the different branches. The overall flow is difficult to read.
2 participants
Description
Implemented Healpix cropping as discussed with three different methods for spatial cover. The code also supports different overlaps between crops provided for the teacher and the student and corrects the bug when passing num_sample > 1. In the config, three different examples were provided on how to set up cropping; it was tested on the Juwels booster and it ran for few epochs.
Checklist before asking for review
./scripts/actions.sh lint./scripts/actions.sh unit-test./scripts/actions.sh integration-testlaunch-slurm.py --time 60