correct the formatting for positive numbers with negative exponents and add additional test #590

adrianmolzon · 2025-08-28T15:45:42Z

Fixing #589 (comment)
I was checking whether a minus sign was present, the formatter will now check that the number is negative when formatting for a negative number

Summary by CodeRabbit

Bug Fixes
- Improved numeric sign handling and width-based truncation in log number formatting so scientific notation and negative values display correctly (exponent-aware truncation, sign determined by numeric value).
Tests
- Added/expanded tests covering small-magnitude scientific notation for positive and negative values to prevent regressions.

…nd add additional test

coderabbitai · 2025-08-28T15:45:53Z

Walkthrough

Change numeric sign handling and truncation in bayes_opt/logger.py to use the numeric value (x < 0) and avoid replacing the leading digit for small scientific-notation values; add unit tests covering positive and negative small-magnitude scientific notation.

Changes

Cohort / File(s)	Summary
Logger formatting logic `bayes_opt/logger.py`	Replace string-based sign detection with numeric check (`x < 0`); set sign and decrement width from the numeric sign; remove the non-decimal truncation path so magnitude truncation occurs only when a decimal point is present; preserve exponent detection/appending and padding logic.
Unit tests `tests/test_logger.py`	Expand tests to cover small-magnitude scientific notation for positive and negative values (`1.11111111e-5` → `"1.111e-05"`, `-1.11111111e-5` → `"-1.11e-05"`), plus existing expectations remain.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant C as Caller
  participant L as logger._format_number

  Note over L: High-level formatting flow

  C->>L: _format_number(x, width, prec)
  alt x < 0
    L->>L: sign = "-"
    L->>L: mag = abs(x)
    L->>L: width = width - 1
  else
    L->>L: sign = ""
    L->>L: mag = x
  end
  L->>L: fmt = format(mag, choose fixed or scientific)
  alt scientific notation (has "e")
    L->>L: split into mantissa and exponent
    L->>L: truncate mantissa only when decimal point present
    L->>L: append exponent (normalized, e.g., e-05)
  else
    L->>L: handle decimal rounding/truncation as before
  end
  L->>L: result = sign + formatted(mantissa [+ exponent])
  L->>L: pad result to requested width
  L-->>C: formatted string

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Assessment against linked issues

Objective	Addressed	Explanation
Fix formatting error where positive scientific-notation numbers with long mantissas and negative exponents get a leading minus replacing the first digit (#589)	✅	Sign handling now uses numeric check so leading digit is preserved for positive small-magnitude scientific values.

Pre-merge checks (5 passed)

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title accurately summarizes the primary fix by highlighting the correction of formatting for positive numbers with negative exponents and notes the addition of a corresponding test, aligning directly with the main changes in the pull request. It is concise, specific to the core update in the logger formatting logic, and avoids extraneous details. Therefore, it meets the guidelines for a clear and focused PR title.
Linked Issues Check	✅ Passed	The modifications in _format_number update sign handling to rely on the numeric comparison x < 0 and remove the improper string-based check, directly addressing the bug reported in issue #589. The expanded tests now validate correct formatting for small positive and negative values in scientific notation with negative exponents, confirming the fix. These changes satisfy the core requirement of preserving the leading digit for positive scientific notation values with negative exponents.
Out of Scope Changes Check	✅ Passed	All alterations in bayes_opt/logger.py and tests/test_logger.py are expressly related to fixing the formatting bug and adding coverage for the edge cases described in the linked issue, with no additional features, refactorings, or unrelated code changes introduced.
Description Check	✅ Passed	The description succinctly references the linked issue and explains that the formatter now checks the numeric value for negativity instead of inspecting string content, which aligns with the changes made in bayes_opt/logger.py. It directly relates to the bug fix without introducing unrelated information, making it sufficiently on-topic for this pull request.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

Poem

I twitch my nose and tidy logs tonight,
No dashes stealing digits from the light.
Tiny e-05s now sparkle true,
Positive and negative—both in view.
A rabbit's hop for fixes done just right.

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 006b979 and 4df7b26.

📒 Files selected for processing (1)

bayes_opt/logger.py (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

bayes_opt/logger.py

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)

GitHub Check: Python 3.11 - numpy >=1.25,<2
GitHub Check: Python 3.10 - numpy >=2
GitHub Check: Python 3.13 - numpy >=2
GitHub Check: Python 3.12 - numpy >=1.25,<2
GitHub Check: Python 3.9 - numpy >=2
GitHub Check: Python 3.9 - numpy >=1.25,<2
GitHub Check: Python 3.12 - numpy >=2
GitHub Check: Python 3.11 - numpy >=2
GitHub Check: Python 3.10 - numpy >=1.25,<2

✨ Finishing Touches

📝 Generate Docstrings

🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment

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.}

codecov · 2025-08-28T15:49:42Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.89%. Comparing base (14786ff) to head (4df7b26).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #590      +/-   ##
==========================================
+ Coverage   97.80%   97.89%   +0.08%     
==========================================
  Files          10       10              
  Lines        1186     1185       -1     
==========================================
  Hits         1160     1160              
+ Misses         26       25       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

bayes_opt/logger.py (1)
93-103: Avoid potential exponent duplication when no decimal point exists.

If s contains 'e' but no '.' (rare, but possible with str(x)), result += s[:width] followed by result += end can duplicate the exponent. Minor guard below keeps only the mantissa before appending end.
-            else:
-                result += s[:width]
+            else:
+                # Use only the mantissa when no decimal point is present to avoid duplicating the exponent.
+                head = s[:e_pos] if "e" in s else s
+                result += head[:width]
tests/test_logger.py (1)
90-93: Great regression test covering the reported bug.

This precisely guards against the false-negative formatting for positive numbers with negative exponents. Consider also adding the negative counterpart for symmetry.
     sci_float = 1.11111111e-5
     formatted = logger._format_number(sci_float)
     assert formatted == "1.111e-05"
+
+    # Symmetric negative case
+    sci_float_neg = -1.11111111e-5
+    formatted = logger._format_number(sci_float_neg)
+    assert formatted == "-1.111e-05"

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

MCP integration is disabled by default for public repositories
Jira integration is disabled by default for public repositories
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 14786ff and 56ff0b5.

📒 Files selected for processing (2)

bayes_opt/logger.py (1 hunks)
tests/test_logger.py (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

tests/test_logger.py (1)

bayes_opt/logger.py (1)

_format_number (66-106)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)

GitHub Check: Python 3.12 - numpy >=1.25,<2
GitHub Check: Python 3.9 - numpy >=2
GitHub Check: Python 3.11 - numpy >=1.25,<2
GitHub Check: Python 3.10 - numpy >=2
GitHub Check: Python 3.9 - numpy >=1.25,<2
GitHub Check: Python 3.11 - numpy >=2
GitHub Check: Python 3.13 - numpy >=2
GitHub Check: Python 3.12 - numpy >=2
GitHub Check: Python 3.10 - numpy >=1.25,<2

🔇 Additional comments (1)

bayes_opt/logger.py (1)

85-89: Correct fix for false negatives from exponent substrings.

Switching to x < 0 removes the accidental sign detection from 'e-XX' and resolves the reported formatting bug. Looks good.

adrianmolzon · 2025-08-28T15:56:35Z

@LiemuR Thanks again for pointing out this mistake. To be clear, this was just a formatting issue, but the formatting issue has now been fixed.

@till-m Can I get an approval here

till-m

I'd be fine with merging this as-is, but maybe check if the clanker had some nitpicks worth considering.

till-m

LGTM, but I think it makes sense to add coverage of this little bit of added code.

bayes_opt/logger.py

correct the formatting for positive numbers with negative exponents a…

56ff0b5

…nd add additional test

coderabbitai bot reviewed Aug 28, 2025

View reviewed changes

adrianmolzon mentioned this pull request Aug 28, 2025

Output formatting error for positive numbers in scientific notation with long mantissas and negative exponents #589

Closed

till-m approved these changes Aug 28, 2025

View reviewed changes

add case for negative exponent truncation, add guard for the mantissa

006b979

till-m reviewed Sep 5, 2025

View reviewed changes

bayes_opt/logger.py Outdated Show resolved Hide resolved

delete unnecessary else statement

4df7b26

till-m merged commit c410d51 into bayesian-optimization:master Sep 9, 2025
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

correct the formatting for positive numbers with negative exponents and add additional test #590

correct the formatting for positive numbers with negative exponents and add additional test #590

Uh oh!

adrianmolzon commented Aug 28, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Aug 28, 2025 •

edited

Loading

Uh oh!

codecov bot commented Aug 28, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

adrianmolzon commented Aug 28, 2025

Uh oh!

till-m left a comment

Uh oh!

till-m left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

correct the formatting for positive numbers with negative exponents and add additional test #590

correct the formatting for positive numbers with negative exponents and add additional test #590

Uh oh!

Conversation

adrianmolzon commented Aug 28, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Assessment against linked issues

Pre-merge checks (5 passed)

Poem

Uh oh!

codecov bot commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

adrianmolzon commented Aug 28, 2025

Uh oh!

till-m left a comment

Choose a reason for hiding this comment

Uh oh!

till-m left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adrianmolzon commented Aug 28, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Aug 28, 2025 •

edited

Loading

codecov bot commented Aug 28, 2025 •

edited

Loading

till-m left a comment •

edited

Loading