Skip to content

Conversation

@maciej-flexcompute
Copy link
Collaborator

added quality controls for snap and smooth: zmetric (aka badness) an feature edge deduplication tolerance

Copy link
Collaborator

@piotrkluba piotrkluba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few comments to improve consistency with other metrics

le=1,
description="Error reduction factor. Used in combination with n_smooth_scale. Must be between 0 and 1.",
)
zmetric_threshold: pd.NonNegativeFloat = pd.Field(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do those parameters have maximum values? If so set maximum values using le or write a validator. Add option to disable using False

"errorReduction": (
quality_settings.error_reduction if quality_settings.error_reduction else 0
),
"zMetricThreshold": quality_settings.zmetric_threshold,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add values that disable those metrics using Literal[False]

snappy.QualityMetrics(max_internal_skewness=-2 * u.deg)

snappy.QualityMetrics(max_boundary_skewness=23 * u.deg, max_internal_skewness=89 * u.deg)
with pytest.raises(pd.ValidationError):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add "free" lines that will not throw an error to ensure normal usage is fine.

"minTriangleTwist": -1,
"nSmoothScale": 4,
"errorReduction": 0.75,
"zMetricThreshold": 0.8,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you allow turning off this metric through False, also change the values here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants