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

General Regression Analysis Wrapper #125

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

General Regression Analysis Wrapper #125

wants to merge 9 commits into from

Conversation

drewrip
Copy link
Collaborator

@drewrip drewrip commented Jan 26, 2024

No description provided.

@kpouget
Copy link
Collaborator

kpouget commented Jan 29, 2024

thanks @drewrip ,

could you add a pydantic model to do define the regression result object?

the way I prefer to generate the object looks like this: (model definition, object generation)


def generate_lts_payload(results, import_settings, must_validate=False):

    lts_metadata = generate_lts_metadata(results, import_settings)
    lts_results = generate_lts_results(results)

    # ---

    lts_payload = types.SimpleNamespace()
    lts_payload.__dict__["$schema"] = f"urn:rhods-notebooks-perf:{models_lts.VERSION}"
    lts_payload.metadata = lts_metadata
    lts_payload.results = lts_results

    lts_payload.kpis = lts.generate_lts_kpis(lts_payload)
    lts_payload.regression_results = results.regression_results

    # ---

    json_lts = json.dumps(lts_payload, indent=4, default=functools.partial(json_dumper, strict=False))
    parsed_lts = json.loads(json_lts)

    return models.lts.Payload.parse_obj(parsed_lts)

can you make regression a package (a directory), and ZScoreIndicator a module (a file), and everything in __init__.py.
Not sure if we want the not implemented classes or not 🤔

Comment on lines 142 to 154
if abs(z_score) > self.threshold:
return RegressionStatus(
1,
direction=1 if z_score > 0 else -1,
explanation="z-score greater than threshold",
details={"threshold": self.threshold, "zscore": z_score}
)
else:
return RegressionStatus(
0,
explanation="z-score not greater than threshold",
details={"threshold": self.threshold, "zscore": z_score}
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you try to have a single return here, and use variables to save the explanation/status
that will make the code more readable (=always return a RegressionStatus, but if above threshold, set a different status/explanation)

Comment on lines 112 to 115
self.regression_test(
curr_values,
lts_values
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you save the result of this call into a variable, and use the variable here?
this is a key the important part of the function body, shouldn't be "hidden" in the middle of the object creation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure I'll make those more clear

Copy link
Collaborator Author

@drewrip drewrip Jan 29, 2024

Choose a reason for hiding this comment

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

@kpouget How do you mean: and everything in __init__.py?

Copy link
Collaborator

Choose a reason for hiding this comment

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

in the regression directory, you'll create some modules/files,
and what does not belong to a module, you put it in regression/__init__.py, so that it's reachable with eg regression.RegressionStatus(

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.

None yet

2 participants