Skip to content

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

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.

3 participants