test/normalize_median - #32
Open
leventetn wants to merge 3 commits into
Open
Conversation
Documentation build overview
56 files changed ·
|
idf-io
requested changes
Jul 3, 2026
idf-io
left a comment
idf-io
left a comment
Collaborator
There was a problem hiding this comment.
Please review and address my comments. Only the ones with checkboxes need to be adressed.
| adata, | ||
| log_space=True, | ||
| ) | ||
|
|
Collaborator
There was a problem hiding this comment.
- Missing complementary functionality of "linear space detection".
- In addition please cover that when detected input spaces and passed space differ and force is passed, no error os returned
|
|
||
| class TestNormalizeMedian: | ||
| """Contract tests for normalize_median.""" | ||
|
|
Collaborator
There was a problem hiding this comment.
Please include a succinct description of the testing approach as an holistic intro. It should allow for a quick understanding of where to categorically look for a specific test and if a more general conceptual testing approach is included. Improves readability.
E.g.
- Linear space validation
- Log space validation
- More explanation if relevant for holistic understanding
- Approach x: e.g. (not relevant) due to probabilistic nature of the output data points, a bayes factor approach is used to measure ... under a threshold.
- Validate param fill_na
- Edge cases:
- concept 1
- concept n
- Verbose validation
- Input parameter validation
| log_space=True, | ||
| ) | ||
|
|
||
| # ── G. Verbose output ──────────────────────────────────────────── |
Collaborator
There was a problem hiding this comment.
Missing validation of parameters:
-
fill_naandzero_to_na-> actually I found this one under the "Edge cases" section (test_zero_to_na_persists_and_ignores_zero_in_medianandtest_fill_na_persists_before_normalization). This highlights the value of a good test class docstring overview.
| ) | ||
|
|
||
| @pytest.mark.parametrize("inplace", [True, False]) | ||
| def test_zero_to_na_persists_and_ignores_zero_in_median( |
Collaborator
There was a problem hiding this comment.
I would have done two separate tests, one to validate zero_to_na and another to ensure correct nan handling in the function. This could highlight the tested functionalities more for and easier finding but this is also fine.
| ) | ||
|
|
||
| @pytest.mark.parametrize("inplace", [True, False]) | ||
| def test_fill_na_persists_before_normalization(self, inplace): |
Collaborator
There was a problem hiding this comment.
Same as comment above, I would have made this two different tests but this is also fine. Don't spend time on refactoring.
| np.testing.assert_allclose( | ||
| factors.loc[1:, "shift_log"].to_numpy(), | ||
| [1.0, -1.0], | ||
| ) |
Collaborator
There was a problem hiding this comment.
- Also test that when a sample is all
nan, yielding a sample-levelnanvalue, that the target=max parameter works as expected. The function computing the max could have a different handling ofnancompared to the median function.
| assert factors is adata_out.uns["median_norm"] | ||
| assert "median_norm" not in adata.uns | ||
| adata_out.X[0, 0] = -100.0 | ||
| assert adata.X[0, 0] == 18.0 |
Collaborator
There was a problem hiding this comment.
- You missed a crucial edge case, which is easy to miss if you haven't worked with math programming much ;)
Division by zero can be problematic because it can yieldnanor±infdepending on the numerator. In our case this can happen when thenormalize_median(log_space=False)function computes a sample-level median == 0. Because this is very rare and biologically difficult to handle, I implemented the function to raise an error, prompting the user to handle the input data beforehand.
In the future please be extra careful when you see mathematical operations such as division and logarithms (also as part of more complex functions such as the variance which includes a division).
Implemented in tests/pp/test_normalize_median.py] What changed: Expanded the module docstring into a short holistic testing intro. Renamed the detection section to F. Input-space detection. Added target="max" coverage with an all-NaN sample-level median. Added linear-space detection success coverage. Added force=True coverage for linear-like data passed as log-space. Strengthened the zero-median linear-space error test to cover both inplace modes and confirm no partial mutation or factor storage occurs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.