Skip to content

Conversation

@JATAYU000
Copy link
Contributor

Metadata

@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 10.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.59%. Comparing base (bd8ae77) to head (e7006d8).

Files with missing lines Patch % Lines
openml/tasks/split.py 10.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1567      +/-   ##
==========================================
- Coverage   79.14%   78.59%   -0.56%     
==========================================
  Files          36       36              
  Lines        4320     4330      +10     
==========================================
- Hits         3419     3403      -16     
- Misses        901      927      +26     

☔ 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.

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Question: have you considered inheriting from OpenMLBase, or at least move the __repr__ related logic to a common place, instead of writing a new one?

Not saying that this is how it has to be done, I would like to hear your rationale.

@JATAYU000
Copy link
Contributor Author

Question: have you considered inheriting from OpenMLBase, or at least move the __repr__ related logic to a common place, instead of writing a new one?

OpenMLSplit is independent, so making it inherit OpenMLBase just to reuse two methods did not feel right. I hadn’t considered moving __repr__ to a common place before, now that I think about it creating a small Mixin class would be better? ex ReprMixin which defines __repr__ and _apply_repr_template which expects the class inheriting from it implement _get_repr_body_fields
What is your opinion on that?

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.

Add __repr__ Methods to OpenMLSplit

3 participants