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

chore: Remove filterwarnings from tests for cross-version pandas compatibility #3522

Merged
merged 1 commit into from
Aug 10, 2024

Conversation

MarcoGorelli
Copy link
Contributor

@MarcoGorelli MarcoGorelli commented Aug 5, 2024

pandas has two month-related offsets:

When doing a monthly group-by, I think the first one gives the results that people are usually actually looking for, and is the same across versions, hence removing the need for filterwarnings or version checks

In the bump_chart.py example:

  • grouping by '6MS' means that the windows are:
    • [2000-01-01, 2000-07-01)
    • [2000-07-01, 2001-01-01)
    • ...
  • grouping by '6M' (or, now, '6ME') means that the windows are:
    • (1999-07-31 23:59:59.999999999, 2000-01-31 23:59:59.999999999]
    • (2000-01-31 23:59:59.999999999, 2000-07-31 23:59:59.999999999]
    • (2000-07-31 23:59:59.999999999, 2001-01-31 23:59:59.999999999]

I think the former is a much more natural way of thinking about time series, and so is better suited to the examples anyway (in addition to helping avoid filterwarnings


Output:

before

image

after

image

Due to the explanation above, I think the second one would align more closely without how people would read the chart

@MarcoGorelli MarcoGorelli changed the title chore: remove filterwarnings from tests for cross-version pandas compatibility chore: Remove filterwarnings from tests for cross-version pandas compatibility Aug 5, 2024
@MarcoGorelli MarcoGorelli marked this pull request as ready for review August 5, 2024 12:43
@dangotbanned
Copy link
Member

@MarcoGorelli I tried fixing this in 3e07902 (#3431) which might be what led you here. IIRC the warning suggested 'ME'.

Is this because 'MS' was always an alias, but 'ME' was a newer introduction?

@MarcoGorelli
Copy link
Contributor Author

Yup, that's right

The warning suggests 'ME' as that preserves the 'M' behaviour - but my claim is that people almost certainly didn't really want 'M' in the first places and should've been using 'MS'. I'll write a blog post about this soon ✍️

Copy link
Member

@dangotbanned dangotbanned left a comment

Choose a reason for hiding this comment

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

This removes the need for @pytest.mark.filterwarnings I introduced back in 3e07902 (#3431)

@dangotbanned dangotbanned merged commit ae7e702 into vega:main Aug 10, 2024
21 checks passed
@dangotbanned
Copy link
Member

Thanks @MarcoGorelli

I already reviewed earlier this week, but appreciate the extra visual 👍

If you come across any other pandas-related quirks in the examples, another thing to check is if it comes from https://vega.github.io/vega-lite/examples/

This one seems to be an altair original, so this change works

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants