Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

559 replace does not accept falsy strings or numbers #562

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

babennettdev
Copy link

Overview

Fix for Issue #559 .

  • Series.replace() accepts 0 and '' for both oldValue and newValue.
  • Series.replace() accepts NaN for newValue.
  • Series.replace() throws if oldValue is NaN (notifies user that Series.fillNa() should be used instead).
  • DataFrame.replace() accepts 0 and '' for both oldValue and newValue.
  • DataFrame.replace() accepts NaN for newValue.
  • DataFrame.replace() throws if oldValue is NaN (notifies user that DataFrame.fillNa() should be used instead).
  • Updated unit tests for Series.replace().
  • Updated unit tests for DataFrame.replace().

@risenW risenW requested a review from steveoni March 17, 2023 11:29
@ghost
Copy link

ghost commented Jul 10, 2023

Hi, I've been checking a bit the pull request and it seems that there is a failure in the treatment of dates, this is the code that fails and what it expects, I don't know if it is due to the changes in the pull request or other changes.

Code:

test('DataFrame Dates', () => {
  let data = [
    ['Alice', 2, new Date('2029-01-01 01:00:00')],
    ['Bob', 5, new Date('2019-01-02')],
    ['Charlie', 30, new Date('2020-01-03 01:00:20')],
    ['Dennis', 89, new Date('2022-02-04 02:16:00')]
  ]
  let columns = ["Name", "Count", "Date"];
  let df = new dfd.DataFrame(data, { columns: columns });
  expect(df["Date"].dt.hours().values).toStrictEqual([1, 0, 1, 2]);
})

Error:

Error: expect(received).toStrictEqual(expected) // deep equality

- Expected  - 1
+ Received  + 1

  Array [
    1,
-   0,
+   1,
    1,
    2,
  ]

Regards ✌

Copy link
Member

@risenW risenW left a comment

Choose a reason for hiding this comment

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

Nice, thanks for the fix! Just need to look at why test is failing

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.

2 participants