Skip to content

Conversation

MoritzPotthoffQC
Copy link
Contributor

@MoritzPotthoffQC MoritzPotthoffQC commented Sep 16, 2025

Merge after #150

Motivation

The possible values are now stored as pl.Series (#138), which can't be json-serialized and break repr as the check against the default argument now produces a series object (element-wise comparison) which can't be evaluated as boolean. This means that in these methods, we always treat categories as list (even when they are actually a pl.Series). I am not sure if this has any undesired effects.

Changes

  • Convert the values to a python list before serialization and evaluation in __repr__
  • Extended tests

@MoritzPotthoffQC MoritzPotthoffQC self-assigned this Sep 16, 2025
@github-actions github-actions bot added the fix label Sep 16, 2025
Copy link

codecov bot commented Sep 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (56b2f86) to head (bb4fc17).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #147   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           50        50           
  Lines         2851      2841   -10     
=========================================
- Hits          2851      2841   -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@MoritzPotthoffQC MoritzPotthoffQC changed the title fix: Fix serialization of dy.Enum columns fix: Fix serialization and printing of dy.Enum columns Sep 16, 2025
@MoritzPotthoffQC MoritzPotthoffQC marked this pull request as ready for review September 16, 2025 17:03
@MoritzPotthoffQC
Copy link
Contributor Author

@borchero @AndreasAlbertQC this fixes the current state, but is maybe also an extra argument for this approach. Up to you how we fix it. I am also not sure whether there are any other similar issues left that we did not run into yet.

Copy link
Member

@borchero borchero left a comment

Choose a reason for hiding this comment

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

This is the second issue that we've run into after using Series instead of list here, let's please store the categories as list 😅 fyi @AndreasAlbertQC

EDIT: Sorry, didn't see your additional comment @MoritzPotthoffQC, I agree

@AndreasAlbertQC
Copy link
Collaborator

Could not agree more

@AndreasAlbertQC AndreasAlbertQC changed the base branch from main to enum-no-series September 23, 2025 08:21
@AndreasAlbertQC AndreasAlbertQC changed the title fix: Fix serialization and printing of dy.Enum columns ci: Add tests for serialization and printing of dy.Enum columns Sep 23, 2025
@AndreasAlbertQC
Copy link
Collaborator

This PR is now based on #150, which fixes the underlying problems. I still want to merge this PR to get the tests.

Base automatically changed from enum-no-series to main September 23, 2025 10:08
@github-actions github-actions bot added the ci label Sep 23, 2025
@AndreasAlbertQC AndreasAlbertQC merged commit fbecd10 into main Sep 23, 2025
22 checks passed
@AndreasAlbertQC AndreasAlbertQC deleted the fix-enum-serialization branch September 23, 2025 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants