Rename references to degree of freedom from df to deg_of_free #1056

mainkhan · 2023-10-27T03:45:17Z

Fixes Issue #1030

This PR renames references to degrees of freedom in a chi-squared calculation to deg_of_free to not get confused with when we use df to refer to a data frame.

I have run the unit test suite and verified that the 1155 tests are running without fail.
I have also run pre-commit on all the files without issue.

CLAassistant · 2023-10-27T03:45:28Z

All committers have signed the CLA.

taylorfturner · 2023-10-27T12:49:48Z

@mainkhan if you don't mind, let's open this PR into dev and not main. Thanks for the contribution!

taylorfturner

open PR into dev

mainkhan · 2023-10-27T13:32:30Z

Hey @taylorfturner I changed the base branch. Sorry about that, I know it should be obvious but I think I got confused because in CONTRIBUTING.md the first step to create a pull request is to create a feature branch off of main

taylorfturner · 2023-10-27T13:45:26Z

Hey @taylorfturner I changed the base branch. Sorry about that, I know it should be obvious but I think I got confused because in CONTRIBUTING.md the first step to create a pull request is to create a feature branch off of main

Totally understandable -- we will update that for clarity. Poor direction on our part. Thanks for the contribution!

taylorfturner · 2023-10-27T16:57:52Z

dataprofiler/tests/profilers/test_categorical_column_profile.py

since this is changing behavior of the output for end users

we will need to be very clear in our communication about what changed and when (in PR notes)

also will need to ensure documentation is updated properly to ensure that is up to date and matching code's current state with this change

@mainkhan my one thought is that we should

research needed documentation changes here

if any needed changes open up a PR into dev-gh-pages in parallel to the code change

I looked in to the docs and could not find any information documenting explicitly what the output will be so I'm not sure we need to change any existing documentation. Perhaps just note in release notes documenting that a field in the dict has changed

I see here: https://capitalone.github.io/DataProfiler/docs/0.10.5/html/dataprofiler.dp_logging.html?highlight=chi#dataprofiler.dp_logging.get_child_logger

dataprofiler.profilers.profiler_utils.perform_chi_squared_test_for_homogeneity(categories1: dict, sample_size1: int, categories2: dict, sample_size2: int) → dict[str, int | float | None] Perform a Chi Squared test for homogeneity between two groups. Parameters categories1 (dict) – Categories and respective counts of the first group sample_size1 (int) – Number of samples in first group categories2 (dict) – Categories and respective counts of the second group sample_size2 (int) – Number of samples in second group Returns Results of the chi squared test Return type dict

the documentation only specifies that we are returning a dict but does not define the dict's keys.

more like this area of the docs

Created a companion documentation PR #1057

dismissing my own

taylorfturner · 2023-10-30T14:02:24Z

dataprofiler/profilers/numerical_column_stats.py


-        conservative_t = scipy.stats.t(conservative_df)
+        conservative_t = scipy.stats.t(conservative_deg_of_free)


scipy refers to this as df; however, I agree for clarity in this repo a rename would make the code. Its more readable / cause less confusion / less ambiguity and as a result --> easier to maintain

…lone#1056) * change references to degrees of freedom in chi2 from df to deg_of_free * reformated using black pre-commit hook

* Add null ratio to column stats (#1052) * Delay transforming priority_order into ndarray (#1045) In the changed code, we had a mypy error because numpy ndarrays are not compatible with random.Random.shuffle() (expected argument type is MutableSequence[Any]) We fix this by first instantiating priority_order as a list, then shuffling it, then creating an ndarray from it afterwards. * Rename references to degree of freedom from df to deg_of_free (#1056) * change references to degrees of freedom in chi2 from df to deg_of_free * reformated using black pre-commit hook * add_s3_connection_remote_loading_s3uri_feature (#1054) * add_s3_connection_remote_loading_s3uri_feature * pre-commit fix * created S3Helper class and refactored data_utils and unit test * enhanced test_data.py with test_read_s3_uri * enhanced unit tests and refactored is_s3_uri * refactored some unit-tests structure * rename TestCreateS3Client to TestS3Helper * fix directions for contrib branch (#1059) * Feature: Plugins (#1060) * Reservoir sampling (#826) * add code for reservoir sampling and insert sample_nrows options * pre commit fix * add tests for reservoir sampling * fixed mypy issues * fix import to relative path --------- Co-authored-by: Taylor Turner <[email protected]> Co-authored-by: Richard Bann <[email protected]> * plugins loading + preset plugin fetching implementation (#911) * test * Plugin implementation * comments added to functions * plugin test implementation for plugin presets * forgot an import * added None catch * preset plugin test * removing stuff I forgot to delete * snake_case function names * relative path * relative path * made new file for plugin testing * forgot to delete function from old file * now ive fixed if statement * ok this should be it * Plugin testing (#947) * test * plugin test implementation for plugin presets * forgot an import * added None catch * preset plugin test * snake_case function names * relative path * relative path * forgot to delete function from old file * nothing yet, just want this in two different repos * new test for plugins feature and small update to plugin init * pass * didnt want dir to be overwritten * forgot a dir * fix isort pre-commit * reservoir sample * fix imports * fix testing * fix req to match dev --------- Co-authored-by: Rushabh Vinchhi <[email protected]> Co-authored-by: Richard Bann <[email protected]> Co-authored-by: Liz Smith <[email protected]> * version bump (#1064) * empty test --------- Co-authored-by: Suprabhat Gurrala <[email protected]> Co-authored-by: Junho Lee <[email protected]> Co-authored-by: Main Uddin Khan <[email protected]> Co-authored-by: Mohammad Motamedi <[email protected]> Co-authored-by: Rushabh Vinchhi <[email protected]> Co-authored-by: Richard Bann <[email protected]> Co-authored-by: Liz Smith <[email protected]>

mainkhan requested a review from a team as a code owner October 27, 2023 03:45

mainkhan changed the title ~~degree of freedom rename~~ Rename references to degree of freedom from df to deg_of_free Oct 27, 2023

mainkhan changed the base branch from dev to main October 27, 2023 04:31

taylorfturner previously requested changes Oct 27, 2023

View reviewed changes

taylorfturner assigned mainkhan Oct 27, 2023

taylorfturner added the do_not_merge label Oct 27, 2023

mainkhan changed the base branch from main to dev October 27, 2023 13:14

mainkhan requested a review from taylorfturner October 27, 2023 13:32

taylorfturner added good_first_issue Label for a more simple / introductory issue and removed do_not_merge labels Oct 27, 2023

taylorfturner enabled auto-merge (squash) October 27, 2023 13:46

taylorfturner requested review from tyfarnan, micdavis and ksneab7 October 27, 2023 13:46

mainkhan added 2 commits October 27, 2023 10:20

change references to degrees of freedom in chi2 from df to deg_of_free

db35613

reformated using black pre-commit hook

58ea2cc

auto-merge was automatically disabled October 27, 2023 14:30
Head branch was pushed to by a user without write access

mainkhan force-pushed the issue/degree-of-freedom-rename branch from cfa91f1 to 58ea2cc Compare October 27, 2023 14:30

taylorfturner enabled auto-merge (squash) October 27, 2023 14:30

taylorfturner reviewed Oct 27, 2023

View reviewed changes

mainkhan mentioned this pull request Oct 27, 2023

Update docs to reflect renamed output of deg_of_free #1057

Merged

Merge branch 'dev' into issue/degree-of-freedom-rename

1121e70

taylorfturner reviewed Oct 30, 2023

View reviewed changes

taylorfturner approved these changes Oct 30, 2023

View reviewed changes

micdavis approved these changes Oct 30, 2023

View reviewed changes

taylorfturner merged commit e5773a6 into capitalone:dev Oct 30, 2023
4 checks passed

taylorfturner added the Refactor Code that is being modified to improve the library label Oct 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename references to degree of freedom from df to deg_of_free #1056

Rename references to degree of freedom from df to deg_of_free #1056

mainkhan commented Oct 27, 2023 •

edited

Loading

CLAassistant commented Oct 27, 2023 •

edited

Loading

taylorfturner commented Oct 27, 2023

taylorfturner left a comment

mainkhan commented Oct 27, 2023

taylorfturner commented Oct 27, 2023

taylorfturner Oct 27, 2023

taylorfturner Oct 27, 2023

mainkhan Oct 27, 2023

taylorfturner Oct 27, 2023

mainkhan Oct 27, 2023

taylorfturner Oct 30, 2023

taylorfturner Oct 30, 2023


		conservative_t = scipy.stats.t(conservative_df)
		conservative_t = scipy.stats.t(conservative_deg_of_free)

Rename references to degree of freedom from df to deg_of_free #1056

Rename references to degree of freedom from df to deg_of_free #1056

Conversation

mainkhan commented Oct 27, 2023 • edited Loading

CLAassistant commented Oct 27, 2023 • edited Loading

taylorfturner commented Oct 27, 2023

taylorfturner left a comment

Choose a reason for hiding this comment

mainkhan commented Oct 27, 2023

taylorfturner commented Oct 27, 2023

taylorfturner Oct 27, 2023

Choose a reason for hiding this comment

taylorfturner Oct 27, 2023

Choose a reason for hiding this comment

mainkhan Oct 27, 2023

Choose a reason for hiding this comment

taylorfturner Oct 27, 2023

Choose a reason for hiding this comment

mainkhan Oct 27, 2023

Choose a reason for hiding this comment

taylorfturner Oct 30, 2023

Choose a reason for hiding this comment

taylorfturner Oct 30, 2023

Choose a reason for hiding this comment

mainkhan commented Oct 27, 2023 •

edited

Loading

CLAassistant commented Oct 27, 2023 •

edited

Loading