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

Update nominal.py #72

Closed
wants to merge 2 commits into from
Closed

Update nominal.py #72

wants to merge 2 commits into from

Conversation

sksvineeth
Copy link

Added weighted correlation to Cramers as an option

Added to weighted correlation to Cramers as an option
@shakedzy shakedzy linked an issue Jan 17, 2021 that may be closed by this pull request
@shakedzy
Copy link
Owner

Hi @sksvineeth - thanks for this PR. It still requires much more work:

  • first, the code is broken
  • weights needs a default value
  • there should be a consideration of what happens if a some values of weights is NaN
  • there should be a consideration of what happens if the nan_strategy is set to drop, as some values a re trimmed.

@shakedzy
Copy link
Owner

Fixed he first two bullets, the remaining two requires more work

  • first, the code is broken
  • weights needs a default value
  • there should be a consideration of what happens if a some values of weights is NaN
  • there should be a consideration of what happens if the nan_strategy is set to drop, as some values a re trimmed

@shakedzy
Copy link
Owner

Closing PR as it is untouched for too long

@shakedzy shakedzy closed this Jul 14, 2021
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.

cramers_v_weighted
2 participants