Skip to content

Add epsilon adjustment to fold_change and percent_change #71

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.

Sign up for GitHub

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

Open
OBJECT907 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add epsilon adjustment to fold_change and percent_change #71

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/pdex/_math.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,15 +106,15 @@ def bulk_matrix_geometric(


@nb.njit(parallel=True)
def fold_change(x: np.ndarray, y: np.ndarray) -> np.ndarray:
def fold_change(x: np.ndarray, y: np.ndarray, eps=1e-4) -> np.ndarray:
"""Calculates the log2-fold change between two arrays."""
return np.log2(x / y)
return np.log2((x + eps) / (y + eps))
Comment on lines +109 to +111
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Feb 28, 2026

Choose a reason for hiding this comment

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

critical

The introduction of eps is a significant change in behavior that will affect all calculations, not just cases with division by zero. This will cause existing unit tests to fail.

  1. Broken Tests: The existing tests for this function are based on the exact mathematical definition of fold change and will fail with the introduction of eps. The tests must be updated to reflect this new, regularized calculation. Furthermore, new tests should be added to validate the behavior when y contains zeros.
  2. Incomplete Docstring: The function's docstring is now outdated. It should be updated to describe the new eps parameter, its default value, and its purpose in both regularization and preventing division-by-zero errors. An updated docstring is crucial for maintainability and correct usage by other developers.



@nb.njit(parallel=True)
def percent_change(x: np.ndarray, y: np.ndarray) -> np.ndarray:
def percent_change(x: np.ndarray, y: np.ndarray, eps=1e-4) -> np.ndarray:
"""Calculates the change between two arrays."""
return (x - y) / y
return (x - y) / (y + eps)
Comment on lines +115 to +117
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Feb 28, 2026

Choose a reason for hiding this comment

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

critical

Similar to the change in fold_change, adding eps to the denominator here is a breaking change that will cause existing tests to fail.

  1. Broken Tests: The unit tests for percent_change will fail because their expected values do not account for the eps adjustment. The tests need to be updated to align with the new implementation. A new test case for division by zero should also be added.
  2. Incomplete Docstring: The docstring for this function needs to be updated to include information about the eps parameter. This will ensure that future users of this function understand how it handles potential division by zero.



def mwu(
Expand Down