Skip to content

Conversation

@vyagubov
Copy link
Contributor

@vyagubov vyagubov commented Dec 15, 2025

See #1962

  • Replaced numpy with math (available in python by default).
  • No functional changes to the product's behavior are expected.

Summary by CodeRabbit

  • Refactor

    • Replaced reliance on an external numeric library with built-in Python math operations in utility routines to simplify behavior and reduce overhead.
  • Chores

    • Removed an explicit constraint on a numeric dependency in project metadata to allow newer versions.

✏️ Tip: You can customize this high-level summary in your review settings.

@github-actions
Copy link
Contributor

👋 @vyagubov
Thank you for raising your pull request.
Please make sure to add tests and document all user-facing changes.
You can do this by editing the docs files in this pull request.

@coderabbitai
Copy link

coderabbitai bot commented Dec 15, 2025

Walkthrough

This PR removes numpy usage from a JSON utility by switching np.isinf/np.isnan to math.isinf/math.isnan and deletes the explicit numpy = "<2.0.0" constraint from pyproject.toml. No public API changes.

Changes

Cohort / File(s) Summary
JSON utility change
elementary/utils/json_utils.py
Removed numpy import; added math import; replaced np.isinf() and np.isnan() calls with math.isinf() and math.isnan() in inf_and_nan_to_str.
Project configuration
pyproject.toml
Removed the explicit numpy = "<2.0.0" dependency constraint and updated the nearby comment to reflect that the constraint was removed.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Review focus: correctness of math.isinf/math.isnan usage (scalar inputs), and ensuring no remaining numpy references in the module.
  • Files to double-check:
    • elementary/utils/json_utils.py — confirm behavior with edge-case values (e.g., float('nan'), float('inf')).
    • pyproject.toml — ensure dependency removal aligns with project's dependency policy.

Poem

🐰 I swapped out arrays for simpler math,
Quiet numbers now find a calmer path,
Fewer hops in the dependency stack,
A lighter step along the track,
Hooray — a simpler, hop-filled patch! 🥕✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Replace numpy with math' directly and accurately summarizes the main changes: numpy usage has been replaced with Python's math module in json_utils.py and the numpy constraint removed from pyproject.toml.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between b34a1b0 and f2eb9e7.

📒 Files selected for processing (1)
  • elementary/utils/json_utils.py (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • elementary/utils/json_utils.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
pyproject.toml (1)

16-16: Clarify or update the comment about Python version constraints.

The comment mentions "numpy 1.26.x(which was removed)" but still references it as a reason for Python version limits. Since numpy has been removed from dependencies, this is confusing. Either remove the numpy reference entirely or clarify that the Python version constraint is now solely due to dbt v1.9 compatibility.

Apply this diff to clarify the comment:

-# matches the versions supported by dbt as of v1.9, which are probably due to the limits of numpy 1.26.x(which was removed)
+# matches the versions supported by dbt as of v1.9
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 272a40c and b34a1b0.

📒 Files selected for processing (2)
  • elementary/utils/json_utils.py (2 hunks)
  • pyproject.toml (1 hunks)
🔇 Additional comments (2)
elementary/utils/json_utils.py (2)

2-2: LGTM! Clean replacement of numpy with standard library.

The import change from numpy to Python's built-in math module is appropriate and eliminates an external dependency.

Also applies to: 5-5


87-90: LGTM! Functionally equivalent replacement.

The replacement of np.isinf() and np.isnan() with math.isinf() and math.isnan() is correct. Since line 86 confirms obj is a float, the standard library functions provide identical behavior to their numpy counterparts for single values. Verified that numpy has been completely removed from the codebase.

@vyagubov vyagubov mentioned this pull request Dec 15, 2025
@vyagubov
Copy link
Contributor Author

I will upgrade python version to 3.13 in the next PR, if that one goes well.

@NoyaOffer
Copy link
Contributor

Hey @vyagubov ! We're taking a look, would be happy to get the contribution for Python 3.13 as well 🙌

@vyagubov vyagubov temporarily deployed to elementary_test_env December 17, 2025 12:37 — with GitHub Actions Inactive
@vyagubov
Copy link
Contributor Author

@NoyaOffer
I fixed the pre-commit issue.
Could you pls trigger tests once again?

@arbiv arbiv enabled auto-merge (squash) December 18, 2025 08:40
@arbiv arbiv merged commit 3624b8b into elementary-data:master Dec 18, 2025
5 checks passed
@vyagubov vyagubov deleted the remove_numpy branch December 20, 2025 21:01
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