tamnd / tamnd/firepanda

A series with no name reports an empty string where pandas reports None

Open
#435 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Mojo
Stars
1
Forks
0
PR merge metrics
PR metrics pending

Description

`pd.Series([1, 2]).name` is `None`. `fp.Series([1, 2]).name` is `""`. They are different objects, they are different truthiness in only one direction, and they compare unequal, so any test that checks a name against a pandas answer fails on the difference rather than on anything that matters.

## Where it was found

Writing the `cat` namespace ([#432](https://github.com/tamnd/firepanda/pull/432)). `Series.cat.codes` in pandas hands back a series with no name, and so does firepanda, and a test comparing the two had to assert the firepanda answer with a note rather than the pandas one. It is not a categorical problem at all and it turns up wherever a derived series is built.

## Why it was not fixed there

`PyIndex.label` already maps an empty name to `Python.none()` and `PySeries.label` does not, so the two halves of the library already disagree with each other. Changing `PySeries.label` in the accessor would have made the accessor disagree with the rest of the series surface, which is worse than one consistent difference from pandas.

Three tests assert the current answer directly and would have to change with it: `python/tests/test_arith.py:378`, `python/tests/test_construct.py:122` and `python/tests/test_groupby.py:116`. That is a small number and none of them is asserting the empty string on purpose, they are asserting what the library does.

## What the fix is

`PySeries.label` should return `Python.none()` for an empty name, the way `PyIndex.label` does. The question that needs an answer first is whether the Mojo side should carry an optional name rather than a string that means two things, since right now a series named `""` on purpose and a series with no name are the same object, and `fp.Series([1, 2], name="")` cannot be told from `fp.Series([1, 2])`. pandas can tell them apart.

That is the real decision and it is worth making before the one line change, because doing the one line change first makes a series deliberately named `""` start reporting `None`, which is a second wrong answer rather than a fix. A `String` with a validity flag beside it, or an `Optional[String]`, is the shape, and the cost is every place that reads `.name` internally.

## Scope

`Series.name` on read, `Series.name` on write, the `name=` constructor argument, and whatever `rename` does when it lands. `Index.name` already behaves and should be checked rather than changed.

Contributor guide

Open the contributing guide

Research direction

Read PySeries.label alongside PyIndex.label, then trace the Series name= constructor, name setter, and rename path. Check python/tests/test_arith.py:378, python/tests/test_construct.py:122, and python/tests/test_groupby.py:116, plus the existing Index behavior. Resolve how unnamed and explicitly empty names are represented before updating reads, writes, and tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
pandas, python
Domain
data-engineering
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.