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

Injection automation #141

Draft
wants to merge 37 commits into
base: dev
Choose a base branch
from
Draft

Injection automation #141

wants to merge 37 commits into from

Conversation

isherax
Copy link

@isherax isherax commented Mar 14, 2023

No description provided.

Copy link
Contributor

@Anmol-Srivastava Anmol-Srivastava left a comment

Choose a reason for hiding this comment

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

Looks good to me -- could use top-level and function/class docstrings in the format of other parts of the code (e.g. any of the detector implementations are a good example).

Edit: I only point out some of the missing docstrings below.

import noise


def select_random_classes(series):
Copy link
Contributor

Choose a reason for hiding this comment

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

This could use a docstring describing the function with a section for arguments and return value

@@ -0,0 +1,589 @@
import matplotlib.pyplot as plt
Copy link
Contributor

Choose a reason for hiding this comment

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

A top-level docstring describing what this module is for in a short line or so would be helpful

menelaus/injection/injection_automation.py Show resolved Hide resolved
return [class_a, class_b]


class InjectionTesting:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here, a docstring which can be modeled after the docstrings in any of the detectors e.g. KdqTreeBatch


return [start_row, end_row]

def train_linear_model(self, x_cols=None, y_col=None, start=0, end=0.75):
Copy link
Contributor

Choose a reason for hiding this comment

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

Docstring for summary, arguments, output

@Anmol-Srivastava
Copy link
Contributor

It also looks like the GitHub checks are failing because of tests not passing / non-100% coverage and linting/security steps missing.

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