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

53 fix feedback tas feedback re check multicollinearity #58

Merged

Conversation

awlh18
Copy link
Collaborator

@awlh18 awlh18 commented Feb 1, 2025

Addressed the following comments:

  1. "check_multicollinearity" function did not check if input variables are of the correct datatype.

  2. For "test_multicollinearity_wrong_input.py" did not test for if the input variabales are as expected, and if errors are raised when the input variable is not of an expected data type (e.g., expect train_df as dataframe but user uses a list)

  3. ("tests/test_multicollinearity.py", "tests/test_multicollinearity_wrong_input.py") are not well documented.

@awlh18 awlh18 added this to the milestone 4 milestone Feb 1, 2025
@awlh18 awlh18 linked an issue Feb 1, 2025 that may be closed by this pull request
Copy link
Collaborator

@Psingh238 Psingh238 left a comment

Choose a reason for hiding this comment

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

Please check out comments for the new test file.


def test_function_output_incorrect_dtype():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Duplicate function name, would be better to merge them both and test both cases in the same function or renaming the function name to specify what is being tested.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good documentation

Copy link

codecov bot commented Feb 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@5e32bf2). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #58   +/-   ##
=======================================
  Coverage        ?   97.65%           
=======================================
  Files           ?        5           
  Lines           ?      128           
  Branches        ?        0           
=======================================
  Hits            ?      125           
  Misses          ?        3           
  Partials        ?        0           

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

Copy link
Collaborator

@Psingh238 Psingh238 left a comment

Choose a reason for hiding this comment

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

Changes look good. Approved and merged.

@Psingh238 Psingh238 merged commit 17a9b24 into main Feb 3, 2025
6 checks passed
@Psingh238 Psingh238 deleted the 53-fix-feedback-tas-feedback-re-check_multicollinearity branch February 3, 2025 22:37
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.

Fix feedback: TA's feedback re: check_multicollinearity
3 participants