The `clipper_scale_factor` passed in `**conf_extra` is not forwarded to the metafile output

[tik65536]

Carry forward from the issues at LandscapeGeoinformatics/pydggsapi#67 and sahrk/DGGRID#97.

To generate zones at the refinement level of 18, the parameter clipper_scale_factor of DGGRID has to increase to 100_000_000 to generate the correct number of zones. However, when I pass the setting to those grid_cell_xxx functions in the extra_config parameter, the setting doesn't carry forward to the dggs when invoking the update function.

dggrid4py/dggrid4py/dggrid_runner.py

Lines 1319 to 1323 in dc7bd3a

	tmp_id = uuid.uuid4()
	tmp_dir = self.working_dir
	dggs = dgselect(dggs_type = dggs_type, res= resolution, mixed_aperture_level=mixed_aperture_level)
	dggs.update(**conf_extra, strict=True)

dggrid4py/dggrid4py/dggrid_runner.py

Lines 600 to 612 in dc7bd3a

	def update(self, strict=False, **kwargs):
	"""
	Back propagate keyword parameters if they can be mapped to an attribute handled by this class.
	:param strict:
	If True, only keys that match existing attributes will be set.
	If False, unknown keys will be set with provided name.
	"""
	for key, value in kwargs.items():
	found = self._resolve_key(key, strict=strict)
	if found:
	self.set_par(found, value)

The issue is that the setting clipper_scale_factor is not defined in the Dggs data class, so when the update is set with strict=True, it will skip the setting.

[tik65536]

I think adding the clipper_scale_factor to the Dggs data class with a default value of 1000000 will resolve this issue.

Alternatively, if we know the relationship between this parameter and the refinement level, then we can adjust its value within those grid_cells_xxx functions.

[tik65536]

The modification 693cf8b failed the CI test.

FAILED tests/test_dggrid.py::test_cells_for_geo_points - ValueError: some error happened under the hood of dggrid (exit code 1): ** executing DGGRID version 8.42 with GDAL version 3080400 **
type sizes: big int: 64 bits / big double: 128 bits

** using meta file metafile_bb9d174d-b974-4aa9-9fa3-354ae8a7cced...
FATAL ERROR: DgApParamList::setParam() unknown parameter clipper_scale_factor
=================== 5 failed, 9 passed, 13 warnings in 3.59s ===================

With the following metafile config.

   {
        'cell_output_gdal_format FlatGeobuf',
        'cell_output_type GDAL',
        'clip_cell_addresses 023',
        'clip_cell_densification 5',
        'clip_cell_res 1',
        'clip_subset_type COARSE_CELLS',
  +     'clipper_scale_factor 1000000',
        'dggrid_operation GENERATE_GRID',
        'dggs_aperture 7',
        'dggs_orient_specify_type SPECIFIED',
        'dggs_proj ISEA',
        'dggs_res_spec 3',
        'dggs_topology HEXAGON',
        'dggs_type IGEO7',
        'dggs_vert0_azimuth 0.0',
        'dggs_vert0_lat 58.282525588538994675786',
        'dggs_vert0_lon 11.20',
        'input_address_type HIERNDX',
        'output_address_type HIERNDX',
        'output_cell_label_type OUTPUT_ADDRESS_TYPE',
        'point_output_type NONE',
        'precision 7',
    }

However, according to the DGGRID documentation, the parameter clipper_scale_factor should apply to the operation GENERATE_GRID

Because the parameter is for polygons, not for points.

Would I also need to fix the test cases?

[fmigneault]

Since this is a specific clipping parameter, I think the change must happen around here:

dggrid4py/dggrid4py/dggrid_runner.py

Lines 820 to 826 in dc7bd3a

	# clip_subset_types
	if subset_conf['clip_subset_type'] == 'WHOLE_EARTH':
	metafile.append("clip_subset_type " + subset_conf['clip_subset_type'])
	elif subset_conf['clip_subset_type'] in [ 'SHAPEFILE' , 'AIGEN', 'GDAL'] and not subset_conf['clip_region_files'] is None:
	metafile.append("clip_subset_type " + subset_conf['clip_subset_type'])
	metafile.append("clip_region_files " + subset_conf['clip_region_files'])
	elif subset_conf['clip_subset_type'] in [ 'SEQNUMS', 'COARSE_CELLS', 'INPUT_ADDRESS_TYPE'] and not subset_conf['clip_region_files'] is None:

Parameter typings to be considered are already defined here:

dggrid4py/dggrid4py/dggrid_runner.py

Lines 298 to 303 in dc7bd3a

	"clip_subset_type": DggsClipSubsetTypeT,
	"clip_cell_res": int,
	"clip_cell_addresses": str,
	"clip_region_files": str,
	"clip_cell_densification": int,
	"clipper_scale_factor": float,

Just need to apply the missing ones.

Also, since there is quite a bit of repetition in the following functions, I think a dedicated helper function that handles all clip[per]_... parameters consistently would be useful.

dggrid4py/dggrid4py/dggrid_runner.py

Lines 1326 to 1335 in dc7bd3a

	if not clip_geom is None and clip_geom.area > 0:
	clip_gdf = gpd.GeoDataFrame(pd.DataFrame({'id' : [1], 'geometry': [clip_geom]}), geometry='geometry', crs=4326)
	clip_path = Path(tmp_dir) / f"temp_clip_{tmp_id}.{self.tmp_geo_out['ext']}"
	clip_gdf.to_file(str(clip_path), driver=self.tmp_geo_out['driver'])
	subset_conf.update({
	'clip_subset_type': 'GDAL',
	'clip_region_files': str( (Path(tmp_dir) / f"temp_clip_{tmp_id}.{self.tmp_geo_out['ext']}").resolve()),
	})

dggrid4py/dggrid4py/dggrid_runner.py

Lines 1424 to 1433 in dc7bd3a

	if not clip_geom is None and clip_geom.area > 0:
	clip_gdf = gpd.GeoDataFrame(pd.DataFrame({'id' : [1], 'geometry': [clip_geom]}), geometry='geometry', crs=4326)
	clip_path = Path(tmp_dir) / f"temp_clip_{tmp_id}.{self.tmp_geo_out['ext']}"
	clip_gdf.to_file(str(clip_path), driver=self.tmp_geo_out['driver'] )
	subset_conf.update({
	'clip_subset_type': 'GDAL',
	'clip_region_files': str( (Path(tmp_dir) / f"temp_clip_{tmp_id}.{self.tmp_geo_out['ext']}").resolve()),
	})

dggrid4py/dggrid4py/dggrid_runner.py

Lines 1526 to 1532 in dc7bd3a

	seq_df = pd.DataFrame({ input_address_type: cell_id_list})
	seq_df.to_csv( str( (Path(tmp_dir) / f"temp_clip_{tmp_id}.txt").resolve()) , header=False, index=False, columns=[input_address_type], sep=' ')
	subset_conf.update({
	'clip_subset_type': 'SEQNUMS',
	'clip_region_files': str( (Path(tmp_dir) / f"temp_clip_{tmp_id}.txt").resolve()),
	})

dggrid4py/dggrid4py/dggrid_runner.py

Lines 1673 to 1680 in dc7bd3a

	seq_df = pd.DataFrame({ input_address_type: cell_id_list})
	seq_df.to_csv( str( (Path(tmp_dir) / f"temp_clip_{tmp_id}.txt").resolve()) , header=False, index=False, columns=[input_address_type], sep=' ')
	# TODO, for Z3, Z7, ZORDER can potentially also be COARSE_CELLS / aka parent cells?
	subset_conf.update({
	'clip_subset_type': 'SEQNUMS',
	'clip_region_files': str( (Path(tmp_dir) / f"temp_clip_{tmp_id}.txt").resolve()),
	})

dggrid4py/dggrid4py/dggrid_runner.py

Lines 1797 to 1806 in dc7bd3a

	if not clip_geom is None and clip_geom.area > 0:
	clip_gdf = gpd.GeoDataFrame(pd.DataFrame({'id' : [1], 'geometry': [clip_geom]}), geometry='geometry', crs=4326)
	clip_path = Path(tmp_dir) / f"temp_clip_{tmp_id}.{self.tmp_geo_out['ext']}"
	clip_gdf.to_file(str(clip_path), driver=self.tmp_geo_out['driver'] )
	subset_conf.update({
	'clip_subset_type': 'GDAL',
	'clip_region_files': str( (Path(tmp_dir) / f"temp_clip_{tmp_id}.{self.tmp_geo_out['ext']}").resolve()),
	})

[tik65536]

@fmigneault, thanks for pointing out where I should modify the code.

I have created a PR #45 for the changes mentioned above. I referenced other specify_xxx helper functions when I made the changes; however, the clipping helper function requires parameters that are significantly different from those of other helper functions. Is there a more effective way to obtain that input? I would appreciate your comments.

Additionally, AnyGeometry doesn't pass the check when I create the PR on GitHub, but it works fine in my local environment, so in the end, I just followed other parts of the code and changed it to "AnyGeometry"

Thanks,
Tik

[fmigneault]

however, the clipping helper function requires parameters that are significantly different from those of other helper functions

This is fine. The important part is to avoid code duplication. I think it is good that these helpers use more explicit parameters (compared to the other util functions using catch-all **kwargs) since it makes the intent clearer.

AnyGeometry doesn't pass the check

It is because of the TYPE_CHECKING wrapper:

dggrid4py/dggrid4py/dggrid_runner.py

Lines 33 to 38 in dc7bd3a

	if TYPE_CHECKING:
	# place here to avoid imposing 'shapely' if not installed, but supported interface
	from geopandas.base import GeometryArray
	from shapely.geometry.base import BaseGeometry # noqa
	AnyGeometry = BaseGeometry | GeometryArray

Normally, this is set by typing annotation tools like mypy and IDEs, but is not "active" when simply executing the python code (as during pytest calls). This was originally intended that way to allow using the definitions without having these packages installed. However, they are now required dependencies, so you could remove the if TYPE_CHECKING: nesting to make it always defined since they should always be available.

dggrid4py/pyproject.toml

Lines 36 to 37 in dc7bd3a

	shapely = ">=1.8.0, <3.0.0"
	geopandas = ">=0.9.0, <2.0.0"