Skip to content

raise actionable errors for missing DA parameters - #894

Open
mvanhorn wants to merge 1 commit into
PythonOT:masterfrom
mvanhorn:fix/528-da-required-parameter-validation
Open

mvanhorn wants to merge 1 commit into
PythonOT:masterfrom
mvanhorn:fix/528-da-required-parameter-validation

Conversation

@mvanhorn

Copy link
Copy Markdown

Types of changes

Fitting a domain-adaptation estimator with a required sample matrix left unset still looks successful, so later code treats the model as fitted. Transporting samples or labels with a required input left unset comes back empty. The only report is a printed parameter name, which is easy to miss until a later step fails. Fit and transport gate the real work on a helper that returns false when any argument is None and only prints the missing names. Fit then falls through and returns the same object. The transport methods return an array only from inside the successful branch, so a failed check yields no result.

A private helper in the domain-adaptation module raises ValueError and names every missing required input in one message. Base fit and sample or label transports, the linear transports also used by the linear Gromov estimator, the class-regularized Sinkhorn fits, mapping fit and transform, unbalanced Sinkhorn fit, and the multi-source fit, transform, and label transports call it before backend selection and before multi-source fit unpacks its source lists, then always run the previous success path. Labels stay optional except where a fit already required them. The shared boolean parameter check is unchanged, and the docstrings, release notes, and tests expect this exception instead of a printed warning or an incomplete unbalanced fit that still sets the backend.

Fixes #528

Motivation and context / Related issue

A private helper in the domain-adaptation module raises ValueError and names every missing required input in one message. Base fit and sample or label transports, the linear transports also used by the linear Gromov estimator, the class-regularized Sinkhorn fits, mapping fit and transform, unbalanced Sinkhorn fit, and the multi-source fit, transform, and label transports call it before backend selection and before multi-source fit unpacks its source lists, then always run the previous success path. Labels stay optional except where a fit already required them. The shared boolean parameter check is unchanged, and the docstrings, release notes, and tests expect this exception instead of a printed warning or an incomplete unbalanced fit that still sets the backend.

How has this been tested (if it applies)

Ran make test locally on macOS for the POT distribution_estimation_uniform change; it fails the same way on the base branch, so the failure predates this change (it fails the same way on upstream/master).
Tests for the POT distribution_estimation_uniform change live in test/test_da.py.

PR checklist

  • I have read the CONTRIBUTING document.
  • The documentation is up-to-date with the changes I made (check build artifacts).
  • All tests passed, and additional code has been covered with new tests.
  • I have added the PR and Issue fix to the RELEASES.md file.
    Nothing in the POT distribution_estimation_uniform change to ot/da.py backs it.

A private helper in the domain-adaptation module raises ValueError and
names every missing required input in one message. Base fit and sample
or label transports, the linear transports also used by the linear
Gromov estimator, the class-regularized Sinkhorn fits, mapping fit and
transform, unbalanced Sinkhorn fit, and the multi-source fit, transform,
and label transports call it before backend selection and before
multi-source fit unpacks its source lists, then always run the previous
success path. Labels stay optional except where a fit already required
them. The shared boolean parameter check is unchanged, and the
docstrings, release notes, and tests expect this exception instead of a
printed warning or an incomplete unbalanced fit that still sets the
backend.

Fixes PythonOT#528

This branch has not been deployed

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stricter check_params protocol for DA module would simplify debugging

1 participant