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.

Sign up for GitHub

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

Jump to bottom

fix: now mode returns a list #396

Merged
merged 6 commits into from
Jan 31, 2023
Merged

fix: now mode returns a list #396

merged 6 commits into from
Jan 31, 2023

Conversation

Gerhardsa0
Copy link
Contributor

@Gerhardsa0 Gerhardsa0 commented Jan 31, 2023 •
edited
Loading

Closes #395 .

Summary of Changes

now mode returns a list of the most common elements

changed in imputer that the first element of the list is taken but maybe we want to change this

adjusted tests and fixed test_summary

@Gerhardsa0
now mode returns a list
e6a6e2c 
changed in imputer that the first element of the list is taken but maybe we want to change this

adjusted tests and fixed test_summary
@Gerhardsa0 Gerhardsa0 linked an issue Jan 31, 2023 that may be closed by this pull request
mode: return a list with all values that occur as often as the most common value #395
Closed
@github-actions
Copy link

github-actions bot commented Jan 31, 2023 •
edited
Loading

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ PYTHON bandit 5 0 0.58s
✅ PYTHON black 5 0 0 0.67s
✅ PYTHON flake8 5 0 0.41s
✅ PYTHON isort 5 0 0 0.25s
✅ PYTHON mypy 5 0 1.87s
✅ PYTHON pylint 5 0 1.98s

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

Gerhardsa0 referenced this pull request Jan 31, 2023
@SmiteDeluxe
fix: Minor fixes for visualization tutorial, summary & heatmap (#391)
4926613 
### Summary of Changes

Fixed the visualization tutorial with discussed changes from #380 as
well as 3 of the bullet points from #381 (namely summary header, summary
all string and heatmap color scheme).

---------

Co-authored-by: SmiteDeluxe <[email protected]>
@codecov
Copy link

codecov bot commented Jan 31, 2023 •
edited
Loading

Codecov Report

Merging #396 (b85e3e4) into main (4c8d453) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #396      +/-   ##
==========================================
+ Coverage   80.76%   80.77%   +0.01%     
==========================================
  Files         180      180              
  Lines        6399     6403       +4     
  Branches     1293     1293              
==========================================
+ Hits         5168     5172       +4     
  Misses        856      856              
  Partials      375      375
Impacted Files Coverage Δ
Runtime/safe-ds/safeds/data/tabular/_column.py 89.51% <100.00%> (ø)
...-ds/safeds/data/tabular/transformation/_imputer.py 98.03% <100.00%> (+0.16%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Gerhardsa0
Copy link
Contributor Author

@lars-reimann right now the Imputer just takes the first elements of the mode list, should we change it?
I am not sure what behavior is wanted so I am asking here.

@Gerhardsa0 Gerhardsa0 changed the title now mode returns a list fix: now mode returns a list Jan 31, 2023
WinPlay02
WinPlay02 previously approved these changes Jan 31, 2023
View reviewed changes
@lars-reimann
Copy link
Member

@lars-reimann right now the Imputer just takes the first elements of the mode list, should we change it? I am not sure what behavior is wanted so I am asking here.

For now, I would raise an exception in this case. The user can then use the Constant strategy to specify the exact value.

@Gerhardsa0 Gerhardsa0 requested a review from WinPlay02 January 31, 2023 15:02
@Gerhardsa0 Gerhardsa0 merged commit e567e7a into main Jan 31, 2023
@Gerhardsa0 Gerhardsa0 deleted the 395-mode-return-a-list-with-all-values-that-occur-as-often-as-the-most-common-value branch January 31, 2023 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WinPlay02 WinPlay02 WinPlay02 approved these changes

@lars-reimann lars-reimann Awaiting requested review from lars-reimann lars-reimann is a code owner

@SmiteDeluxe SmiteDeluxe Awaiting requested review from SmiteDeluxe

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

mode: return a list with all values that occur as often as the most common value
3 participants
@Gerhardsa0 @lars-reimann @WinPlay02