-
Notifications
You must be signed in to change notification settings - Fork 9
Consolidate clipping parameters handling in a helper function #45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
tik65536
wants to merge
10
commits into
allixender:master
Choose a base branch
from
LandscapeGeoinformatics:consolidate_clipping_parameters_handling_in_helper_function
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Consolidate clipping parameters handling in a helper function #45
tik65536
wants to merge
10
commits into
allixender:master
from
LandscapeGeoinformatics:consolidate_clipping_parameters_handling_in_helper_function
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- added `clipper_scale_factor` to Dggs class
This reverts commit 693cf8b. The fix is not done correctly according to allixender#44 (comment)
- followed the suggestion from allixender#44 (comment) - added the helper function `specify_clip_setting` to handle clip[per] related metafile settings. - But it failed the test case `test_cells_for_geo_points` with cell_ids_only=False.
- fixed the input_address_type is None in `cells_for_geo_points` when the original `output_address_type` is None and zone_id_only is False - changed the value of rf_level_scalefactor dictionary to string instead of int - added test cases for the clipped_scale_factor
-changed the helper function name from `specify_clip_setting` to `specify_clip_settings`
- change the type of clip_geom from AnyGeometry to "AnyGeometry"
fmigneault
reviewed
Dec 18, 2025
Contributor
fmigneault
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
- grouped the handling of clip_subset_type under an if block - moved the clipper_scale_factor under the if block - removed the TYPE_CHECKING for AnyGeometry - changed to use AnyGeometery dtype, instead of "AnyGeometry"
bugfix syntax error when using the dictionary get.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary :
specify_clip_settingsto consolidate clipping parameters handling in eachgrid_cell_xxxfunction,grid_cellids_for_extentandcells_for_geo_pointsat one place.clipper_scale_factoris added according to the resolution (above 16)clipper_scale_factorcomes from a static dictionary; those values come from the checking onDGGRIDby @fmigneault in the issue The number of zones generated of IGEO7 at refinement level 18 doesn't matched with expected. sahrk/DGGRID#97 (comment):Related issue in DGGRID:
sahrk/DGGRID#97 (comment)