Skip to content
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

Re-evaluate the output of .validate() functions #3121

Closed
chukarsten opened this issue Dec 3, 2021 · 1 comment
Closed

Re-evaluate the output of .validate() functions #3121

chukarsten opened this issue Dec 3, 2021 · 1 comment
Assignees
Labels
refactor Work being done to refactor code. tech debt

Comments

@chukarsten
Copy link
Contributor

In a recent refactoring of ARIMA/Prophet, the TargetLeakageDataCheck.validate() was invoked. The invocation looks like:

image

From a dev perspective, this is OK. It's not particularly clear or simple and implies some arcane nested structure being returned for validate() that might be worth refactoring into a class or namedtuple to make a bit simpler.

In #3072 , we discuss a similar refactoring that might help make the OS user experience easier. Completing said story also might allow us to refactor away some of these less-clear portions of our code that require an implied innate knowledge of the return structure.

Acceptance criteria: this story will close after #3072 is addressed. If a different return structure is decided for the .validate() functions of the DataChecks, then closing this story involves refactoring the code from the linked PR and filing an issue to further identify and refactor areas of our code that dig through the output of .validate().

@chukarsten chukarsten added refactor Work being done to refactor code. tech debt labels Dec 3, 2021
@angela97lin angela97lin self-assigned this Jan 7, 2022
@angela97lin
Copy link
Contributor

@chukarsten I don't see the original code in the repo now but after #3072 and the rest of the data check API refactor, validate() no longer returns the same structure: it returns a list instead of a dict. This might make it a bit more difficult to get the exact warning / message we expect but then getting the metadata / action is slightly different. Closing for now, we can reopen using the new structure if we still see area for improvement!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Work being done to refactor code. tech debt
Projects
None yet
Development

No branches or pull requests

2 participants