Skip to content

Add clampvalues and filldown transforms#692

Open
UdayPate wants to merge 2 commits into
petl-developers:masterfrom
UdayPate:feature/clamp-transform
Open

Add clampvalues and filldown transforms#692
UdayPate wants to merge 2 commits into
petl-developers:masterfrom
UdayPate:feature/clamp-transform

Conversation

@UdayPate
Copy link
Copy Markdown

Summary

This PR adds two new data transformation functions to petl:

1. clampvalues(table, field, low, high)

Restricts numeric values in a field to a [low, high] range.

  • Values below low are raised to low
  • Values above high are lowered to high
  • Values within range are unchanged
  • Handles None, floats, negative ranges, and boundary equality

2. filldown(table, *fields)

Propagates the last observed non-None value downward within one or more fields.

  • Supports filling multiple fields in a single call
  • Leading None values remain None until a non-None value is seen
  • Raises FieldSelectionError for missing fields

Changes

  • petl/transform/clamp.py — new file
  • petl/transform/filldown.py — new file
  • petl/test/transform/test_clamp.py — 10 unit tests, all passing
  • petl/test/transform/test_filldown.py — 9 unit tests, all passing
  • petl/transform/__init__.py — registered both functions

Tests

508 passed, 18 skipped (0 failures)

- Implements ClampValuesView following petl's Table subclass pattern
- Handles None values, floats, negative ranges, and boundary equality
- Raises ValueError for invalid ranges (low > high) and missing fields
- 10 unit tests covering normal cases and edge cases
- Implements FillDownView following petl's Table subclass pattern
- Supports filling multiple fields in a single call
- Leading None values remain None until a non-None value is seen
- Raises FieldSelectionError for missing fields
- 9 unit tests covering normal cases and edge cases
@qodo-free-for-open-source-projects
Copy link
Copy Markdown

Review Summary by Qodo

Add clampvalues and filldown data transformation functions

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Add clampvalues transform to restrict numeric field values within range
• Add filldown transform to propagate last non-None value downward
• Implement both transforms following petl's Table subclass pattern
• Register both functions in transform module with comprehensive test coverage
Diagram
flowchart LR
  A["Input Table"] -->|clampvalues| B["Clamped Values<br/>low ≤ val ≤ high"]
  A -->|filldown| C["Filled Down<br/>None → last value"]
  B --> D["Output Table"]
  C --> D
Loading

Grey Divider

File Changes

1. petl/transform/clamp.py ✨ Enhancement +77/-0

Clamp numeric values within specified range

• Implements clampvalues() function to restrict numeric values to [low, high] range
• Creates ClampValuesView class extending Table for lazy evaluation
• Handles None values, floats, negative ranges, and non-comparable types
• Validates that low ≤ high and field exists in header

petl/transform/clamp.py


2. petl/transform/filldown.py ✨ Enhancement +80/-0

Propagate last non-None value downward

• Implements filldown() function to propagate last non-None value downward
• Creates FillDownView class extending Table for lazy evaluation
• Supports multiple fields in single call with independent tracking
• Preserves leading None values until first non-None value encountered
• Raises FieldSelectionError for missing fields

petl/transform/filldown.py


3. petl/transform/__init__.py ⚙️ Configuration changes +4/-0

Register clampvalues and filldown transforms

• Import clampvalues from new clamp module
• Import filldown from new filldown module
• Register both functions for public API access

petl/transform/init.py


View more (2)
4. petl/test/transform/test_clamp.py 🧪 Tests +96/-0

Unit tests for clampvalues transform

• 10 unit tests covering basic clamping behavior (below low, above high, within range)
• Tests for None passthrough, header preservation, and float values
• Edge case tests for low==high, negative ranges, and boundary conditions
• Error handling tests for invalid ranges and missing fields

petl/test/transform/test_clamp.py


5. petl/test/transform/test_filldown.py 🧪 Tests +104/-0

Unit tests for filldown transform

• 9 unit tests covering basic filldown behavior with single and multiple fields
• Tests for header preservation, columns with no None values, and leading None handling
• Edge case tests for all-None columns, single-row tables, and multiple field fills
• Error handling test for missing fields raising FieldSelectionError

petl/test/transform/test_filldown.py


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

qodo-free-for-open-source-projects Bot commented Apr 22, 2026

Code Review by Qodo

🐞 Bugs (4) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Action required

1. Filldown export shadowed 🐞 Bug ≡ Correctness
Description
petl.transform.__init__ imports filldown from the new petl/transform/filldown.py, but later
re-imports filldown from petl/transform/fills.py, overwriting the new symbol. As a result, the newly
added filldown implementation is effectively unused and untested via the public API.
Code

petl/transform/init.py[R14-17]

+from petl.transform.clamp import clampvalues
+
+from petl.transform.filldown import filldown
+ 
Evidence
The PR adds an import for the new filldown, but the module already imports filldown later from
fills.py, which overwrites the earlier name binding. The repo already has a filldown implementation
(and registers it on Table), so the new module is dead code unless the later import is
removed/changed.

petl/transform/init.py[14-37]
petl/transform/fills.py[10-103]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`petl/transform/__init__.py` imports `filldown` twice, and the later import from `petl.transform.fills` overwrites the new `petl.transform.filldown.filldown`. This makes the newly added module effectively unreachable from `import petl as etl`.

### Issue Context
There is already an existing `filldown` implementation in `petl/transform/fills.py` (including `Table.filldown = filldown`). The PR adds a second implementation under the same name.

### Fix Focus Areas
- petl/transform/__init__.py[14-37]
- petl/transform/fills.py[10-103]
- petl/transform/filldown.py[1-80]

### Suggested resolution paths
- **Preferred:** Delete `petl/transform/filldown.py` and remove `from petl.transform.filldown import filldown` from `petl/transform/__init__.py` (keep tests if they are intended to validate the existing `fills.filldown`).
- **Alternative (if you truly want a new implementation):** Stop importing `filldown` from `petl.transform.fills` (or rename one of the functions), then ensure the exported one matches petl conventions (supports `missing=`, supports filling all fields when none specified, uses `petl.compat.next`, and registers `Table.filldown`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Clamp field selection broken 🐞 Bug ≡ Correctness
Description
ClampValuesView uses list(header).index(field), so integer field indices (a standard petl feature)
do not work and a ValueError is raised instead of FieldSelectionError. This diverges from
established transform behavior and breaks callers expecting index-based field selection.
Code

petl/transform/clamp.py[R59-63]

+        # find the index of the target field
+        try:
+            idx = list(header).index(self.field)
+        except ValueError:
+            raise ValueError('field %r not found in header %r' % (self.field, header))
Evidence
petl’s shared field-selection utility asindices() supports both field names and integer indices
and raises FieldSelectionError for invalid selections. clampvalues bypasses this and instead
searches for the given value in the header row, which misinterprets integer indices and changes the
exception type.

petl/transform/clamp.py[54-64]
petl/util/base.py[285-303]
petl/transform/fills.py[105-124]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ClampValuesView.__iter__` resolves the target column via `list(header).index(self.field)`. This fails for integer field indices (e.g. `field=0`) and raises `ValueError` instead of `FieldSelectionError`, which is inconsistent with other petl transforms.

### Issue Context
petl provides `petl.util.base.asindices(hdr, spec)` as the standard way to interpret field specs (names and numeric indices) and to raise `FieldSelectionError` on invalid selection.

### Fix Focus Areas
- petl/transform/clamp.py[54-64]
- petl/util/base.py[285-303]

### Implementation sketch
- Import `asindices` (and `FieldSelectionError`) and resolve the single target index via `idx = asindices(header, self.field)[0]`.
- Let `FieldSelectionError` propagate (or raise it explicitly) rather than raising `ValueError`.
- Update/extend tests to cover index-based selection (e.g. `etl.clampvalues(table, 1, low, high)`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

3. Clamp empty-input StopIteration 🐞 Bug ☼ Reliability
Description
ClampValuesView.__iter__ calls next(it) without handling StopIteration, so clampvalues([]) raises
StopIteration rather than returning an empty iterator like other transforms. This makes clampvalues
brittle on edge-case inputs and inconsistent with existing transform behavior.
Code

petl/transform/clamp.py[R54-57]

+    def __iter__(self):
+        it = iter(self.table)
+        header = next(it)
+        yield header  # pass header row through unchanged
Evidence
Other transforms in the codebase guard header extraction with try/except StopIteration and return
early for empty input. clampvalues currently does not, so empty iterables crash during iteration.

petl/transform/clamp.py[54-57]
petl/transform/fills.py[181-187]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ClampValuesView.__iter__` does `header = next(it)` without a StopIteration guard. If `table` is empty (no header row), iteration raises StopIteration.

### Issue Context
Multiple transforms in `petl/transform/*` catch StopIteration when reading the header and simply return.

### Fix Focus Areas
- petl/transform/clamp.py[54-57]
- petl/transform/fills.py[181-187]

### Implementation sketch
- Wrap header extraction:
 - `it = iter(self.table)`
 - `try: header = next(it) except StopIteration: return`
- Consider using `from petl.compat import next` for consistency with the codebase.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. Clampvalues not Table method 🐞 Bug ⚙ Maintainability
Description
clampvalues is not registered on Table (no Table.clampvalues assignment), unlike other transforms,
so method-chaining APIs won’t work (e.g. etl.wrap(table).clampvalues(...)). This is an API
inconsistency relative to the rest of petl transforms.
Code

petl/transform/clamp.py[R5-39]

+def clampvalues(table, field, low, high):
+    """Clamp numeric values in a field so they never fall below `low` or
+    exceed `high`. Values already within the range are left unchanged.
+    Non-numeric values (e.g. ``None``) are passed through as-is.
+
+    For example::
+
+        >>> import petl as etl
+        >>> table1 = [['id', 'score'],
+        ...           [1, -5],
+        ...           [2, 42],
+        ...           [3, 105],
+        ...           [4, None]]
+        >>> table2 = etl.clampvalues(table1, 'score', 0, 100)
+        >>> table2
+        +----+-------+
+        | id | score |
+        +====+=======+
+        |  1 |     0 |
+        +----+-------+
+        |  2 |    42 |
+        +----+-------+
+        |  3 |   100 |
+        +----+-------+
+        |  4 |  None |
+        +----+-------+
+
+    The `low` and `high` parameters are both inclusive bounds. Raises
+    ``ValueError`` if ``low > high``.
+
+    .. versionadded:: 1.8.0
+
+    """
+    return ClampValuesView(table, field, low, high)
+
Evidence
Transforms in this repo typically assign the function onto Table after definition to support
method chaining. The existing filldown implementation does this; clampvalues does not.

petl/transform/clamp.py[5-39]
petl/transform/fills.py[88-92]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`clampvalues` is exposed via `petl.transform.__init__`, but it is not registered as a `Table` method, which is a common convention across petl transforms.

### Issue Context
For example, `filldown` in `petl/transform/fills.py` assigns `Table.filldown = filldown`.

### Fix Focus Areas
- petl/transform/clamp.py[5-39]
- petl/transform/fills.py[88-92]

### Implementation sketch
- Add `Table.clampvalues = clampvalues` after the function definition (matching existing transform patterns).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment on lines +14 to +17
from petl.transform.clamp import clampvalues

from petl.transform.filldown import filldown

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

1. Filldown export shadowed 🐞 Bug ≡ Correctness

petl.transform.__init__ imports filldown from the new petl/transform/filldown.py, but later
re-imports filldown from petl/transform/fills.py, overwriting the new symbol. As a result, the newly
added filldown implementation is effectively unused and untested via the public API.
Agent Prompt
### Issue description
`petl/transform/__init__.py` imports `filldown` twice, and the later import from `petl.transform.fills` overwrites the new `petl.transform.filldown.filldown`. This makes the newly added module effectively unreachable from `import petl as etl`.

### Issue Context
There is already an existing `filldown` implementation in `petl/transform/fills.py` (including `Table.filldown = filldown`). The PR adds a second implementation under the same name.

### Fix Focus Areas
- petl/transform/__init__.py[14-37]
- petl/transform/fills.py[10-103]
- petl/transform/filldown.py[1-80]

### Suggested resolution paths
- **Preferred:** Delete `petl/transform/filldown.py` and remove `from petl.transform.filldown import filldown` from `petl/transform/__init__.py` (keep tests if they are intended to validate the existing `fills.filldown`).
- **Alternative (if you truly want a new implementation):** Stop importing `filldown` from `petl.transform.fills` (or rename one of the functions), then ensure the exported one matches petl conventions (supports `missing=`, supports filling all fields when none specified, uses `petl.compat.next`, and registers `Table.filldown`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment thread petl/transform/clamp.py
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.

1 participant