Skip to content

Conversation

@farhadmd7
Copy link
Collaborator

No description provided.

Copy link
Member

@VladimirShitov VladimirShitov left a comment

Choose a reason for hiding this comment

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

Thanks! Looks great overall. Please, resolve small comments I left, and use fixtures wherever possible



def test_distances_significance(
def distances_significance_test(
Copy link
Member

Choose a reason for hiding this comment

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

Why renaming? I also prefer starting tests with test_

Copy link
Member

Choose a reason for hiding this comment

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

Ah, because this is not the test. Thanks for changing it then, makes sense!



def test_proportions(target, groups):
def statistical_test_proportions(target, groups):
Copy link
Member

Choose a reason for hiding this comment

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

Let's do it like that:

Suggested change
def statistical_test_proportions(target, groups):
def composition_differences_test(target, groups):



@pytest.fixture
def toy_dataframe():
Copy link
Member

Choose a reason for hiding this comment

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

Could you call it toy_metadata if that's what it it? Not clear what an abstract data frame means

"""Creates a toy distance matrix and conditions array for testing."""
distances = squareform(pdist(np.array([[0, 1], [1, 1], [2, 2], [3, 3]])))
conditions = pd.Series(["control", "control", "case", "case"])
# conditions = np.array(["control", "control", "case", "case"])
Copy link
Member

Choose a reason for hiding this comment

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

Please remove commented code :)


assert isinstance(result, pd.DataFrame), "The result should be a DataFrame."
assert result.shape[0] == 2, "The result should contain metadata for 2 samples."
assert "cell_type" in result.columns, "The result should contain the cell_type column."
Copy link
Member

Choose a reason for hiding this comment

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

In a normal scenario, that's not what we want. Cell type is cell-level information, and we want to extract donor-level metadata. The test checks the correct thing, but could you change the column to something else to be less confusing?

), "There should be a unique color for each unique value in the column."


def test_describe_metadata(toy_dataframe, capsys):
Copy link
Member

Choose a reason for hiding this comment

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

Haha, that's a bit too much. The tested function is just for convenience. But thanks for testing it as well


def test_calculate_average_without_nans():
# no nan
array = np.array(
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest using fixtures and give them readable names :) E.g. integer_matrix (can also be used it testing count data), float_matrix_with_nans, and so on

y_true = np.array([0, 1, 1, 0])
y_pred = np.array([0, 1, 1, 0])

result = evaluate_prediction(y_true, y_pred, task="classification")
Copy link
Member

Choose a reason for hiding this comment

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

Could you test other tasks as well?

2,
2,
), "Filtered distances should have shape (2, 2) after removing missing values."
assert filtered_target.shape[0] == 2, "Filtered target should have 2 values after removing missing values."
Copy link
Member

Choose a reason for hiding this comment

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

You can also check the exact values as you know what to expect in this example



def test_select_random_subset():
distances = np.array([[0, 1, 2], [1, 0, 3], [2, 3, 0]])
Copy link
Member

Choose a reason for hiding this comment

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

Use fixtures wherever possible :) It allows to easily extend tests with new data

Base automatically changed from 68-inconsistent-interface to main January 29, 2025 00:52
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