feat(clp-s): Make lossless float retention the default behaviour for clp-s. by gibber9809 · Pull Request #1321 · y-scope/clp

gibber9809 · 2025-09-23T15:07:41Z

Description

This PR follows up on #1176 by making lossless float retention the default behaviour for clp-s. This should be a fairly easy win, since our benchmarks on that PR showed that the compression ratio and performance overhead were minimal.

We may also want to change #1298 at the same time (or this PR if #1298 gets merged first) since the --retain-float-format flag will no longer be required.

Checklist

The PR satisfies the contribution guidelines.
This is a breaking change and that has been indicated in the PR title, OR this isn't a
breaking change.
Necessary docs have been updated, OR no docs need to be updated.

Validation performed

Manually validated that compression behaviour with and without the --no-retain-float-format flag work as expected.

Summary by CodeRabbit

Refactor
- Renamed the float-retention CLI flag to --no-retain-float-format, inverting its behaviour; the public getter still indicates whether float format is retained but now derives from the negated flag.
- Updated option description to: “Do not store extra information to losslessly decompress floats.”
Chores
- Updated job orchestration to remove the old flag and use the new CLI option.

…now default.

coderabbitai · 2025-09-23T15:07:51Z

Walkthrough

Rename clp-s CLI option from --retain-float-format to --no-retain-float-format, invert the internal boolean to m_no_retain_float_format and have the getter return its negation; remove the old flag from the Python command construction. No other control flow changes.

Changes

Cohort / File(s)	Summary
clp-s CLI flag inversion (C++) `components/core/src/clp_s/CommandLineArguments.cpp`, `components/core/src/clp_s/CommandLineArguments.hpp`	Rename internal flag to `m_no_retain_float_format`; change option name/description to `--no-retain-float-format`; `get_retain_float_format()` now returns `!m_no_retain_float_format`; default remains false.
Job orchestration command update (Python) `components/job-orchestration/job_orchestration/executor/compress/compression_task.py`	Remove `--retain-float-format` from clp-s command invocation; no other command or control flow changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 40.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 accurately and succinctly states the primary change—making lossless float retention the default for clp-s—and matches the changeset (flag rename and flipped default semantics). It is component-scoped, concise, and readable, so a teammate scanning history will understand the main intent. The phrasing avoids noise and clearly identifies the feature change.

✨ Finishing touches

📝 Generate Docstrings

🧪 Generate unit tests

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

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1ea660a and a34b2a5.

📒 Files selected for processing (1)

components/job-orchestration/job_orchestration/executor/compress/compression_task.py (0 hunks)

💤 Files with no reviewable changes (1)

components/job-orchestration/job_orchestration/executor/compress/compression_task.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). (6)

GitHub Check: manylinux_2_28-x86_64-static-linked-bins
GitHub Check: ubuntu-jammy-static-linked-bins
GitHub Check: package-image
GitHub Check: musllinux_1_2-x86_64-static-linked-bins
GitHub Check: ubuntu-jammy-dynamic-linked-bins
GitHub Check: lint-check (ubuntu-24.04)

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

coderabbitai

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 774a106 and 1ea660a.

📒 Files selected for processing (3)

components/core/src/clp_s/CommandLineArguments.cpp (1 hunks)
components/core/src/clp_s/CommandLineArguments.hpp (2 hunks)
components/job-orchestration/job_orchestration/executor/compress/compression_task.py (0 hunks)

💤 Files with no reviewable changes (1)

components/job-orchestration/job_orchestration/executor/compress/compression_task.py

🧰 Additional context used

📓 Path-based instructions (1)

**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}

⚙️ CodeRabbit configuration file

Prefer false == <expression> rather than !<expression>.

Files:

components/core/src/clp_s/CommandLineArguments.hpp
components/core/src/clp_s/CommandLineArguments.cpp

⏰ 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). (15)

GitHub Check: centos-stream-9-dynamic-linked-bins
GitHub Check: centos-stream-9-static-linked-bins
GitHub Check: ubuntu-jammy-static-linked-bins
GitHub Check: package-image
GitHub Check: ubuntu-jammy-dynamic-linked-bins
GitHub Check: ubuntu-jammy-lint
GitHub Check: musllinux_1_2-x86_64-static-linked-bins
GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
GitHub Check: manylinux_2_28-x86_64-static-linked-bins
GitHub Check: build-macos (macos-15, true)
GitHub Check: build-macos (macos-15, false)
GitHub Check: build-macos (macos-14, true)
GitHub Check: lint-check (macos-15)
GitHub Check: lint-check (ubuntu-24.04)

🔇 Additional comments (2)

components/core/src/clp_s/CommandLineArguments.hpp (1)

105-107: Getter inversion LGTM.

Public API preserved while default behaviour flips; style aligns with guidelines.

components/core/src/clp_s/CommandLineArguments.cpp (1)

248-251: Docs/help follow‑through — no occurrences of --retain-float-format found.

rg only matched the new flag declaration (components/core/src/clp_s/CommandLineArguments.cpp:248 — "no-retain-float-format") and a design-doc filename reference (components/core/src/clp_s/FloatFormatEncoding.hpp:16 → docs/src/dev-guide/design-retain-float-format.md). No other references to --retain-float-format detected; update any external docs/orchestration scripts outside this repo if needed.

…clp-s. (y-scope#1321)

gibber9809 added 2 commits September 23, 2025 15:00

Make float format retention default in clp-s

3bb8d51

Remove retain-float-format option in compression_task.py since it is …

1ea660a

…now default.

gibber9809 requested review from a team and wraymo as code owners September 23, 2025 15:07

coderabbitai Bot reviewed Sep 23, 2025

View reviewed changes

Comment thread components/core/src/clp_s/CommandLineArguments.cpp

Comment thread components/core/src/clp_s/CommandLineArguments.hpp

kirkrodrigues approved these changes Sep 23, 2025

View reviewed changes

gibber9809 added 2 commits September 23, 2025 15:22

Merge branch 'main' into default-retain-float-format

a34b2a5

Merge branch 'main' into default-retain-float-format

9e2abd9

kirkrodrigues merged commit 17ec517 into y-scope:main Sep 23, 2025
27 checks passed

junhaoliao pushed a commit to junhaoliao/clp that referenced this pull request May 17, 2026

feat(clp-s): Make lossless float retention the default behaviour for …

aa734c8

…clp-s. (y-scope#1321)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(clp-s): Make lossless float retention the default behaviour for clp-s.#1321

feat(clp-s): Make lossless float retention the default behaviour for clp-s.#1321
kirkrodrigues merged 4 commits into
y-scope:mainfrom
gibber9809:default-retain-float-format

gibber9809 commented Sep 23, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Sep 23, 2025 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

gibber9809 commented Sep 23, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Validation performed

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gibber9809 commented Sep 23, 2025 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Sep 23, 2025 •

edited

Loading