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

Analytics Module Tests #157

Merged
merged 6 commits into from
Jan 19, 2019
Merged

Analytics Module Tests #157

merged 6 commits into from
Jan 19, 2019

Conversation

ADA110
Copy link
Contributor

@ADA110 ADA110 commented Jan 7, 2019

Tests for the analytics class, including feature importance and partial dependence plots. Testing includes checking the presence of a list of features and verifying that certain partial dependence plots are saved in the same folder based on this specific example.

@@ -118,9 +118,9 @@ def fit(self, df, target, **fit_kwargs):
self._features = df.drop(columns=target).columns.tolist()
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, not sure why this adaptor stuff is being changed....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, the logger attribute should not be changed. I thought the presence of the underscore was the reason I was getting issues in the other parts of that file, but it must have been something else. I'll change it back

@ardunn
Copy link
Contributor

ardunn commented Jan 14, 2019

Looks good, thank you! Could you resolve the logger comments I posted? I'll merge after that!

@ADA110
Copy link
Contributor Author

ADA110 commented Jan 17, 2019

@ardunn This one should also be good to go

@ardunn
Copy link
Contributor

ardunn commented Jan 18, 2019

@ADA110 you have conflicting files (see above). Please resolve and I'll go ahead and merge

@ADA110
Copy link
Contributor Author

ADA110 commented Jan 19, 2019

@ardunn merge conflict resolved

@ardunn ardunn merged commit 8e3bfec into hackingmaterials:master Jan 19, 2019
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.

2 participants