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

Fixed bug in House_Prices_Regression_Interpret tutorial #1014

Closed
wants to merge 2 commits into from

Conversation

dbish
Copy link
Contributor

@dbish dbish commented Aug 19, 2022

Described in issue: 1012

Background
There's a line that must have been edited/added at some point that assumes more then one tensor is being returned from lc.attribute, but there's only one (since only one tensor is passed in):

lc_attr_test = lc.attribute(X_test, n_steps=100, attribute_to_layer_input=True)

shape: test_examples x size_hidden

lc_attr_test = lc_attr_test[0]
The second line here of setting lc_attr_test to the 0th index, then sets it to the tensor index instead of the first tensor, which in turn in the next cell means that "lc_attr_test.shape[1]" throws an exception.

Changes
Fix is to just take out the reassigning of "lc_attr_test = lc_attr_test[0]".

Testing
Tested running the notebook before the change (which showed the error described) and after, which produced the plot which was originally shown in the static tutorial page and matches expectations now (see attachment)
Screen Shot 2022-08-18 at 9 59 25 PM
.

@dbish
Copy link
Contributor Author

dbish commented Aug 19, 2022

Looks like this tutorial hasn't been updated in a bit, it works now, but there are a few new warnings that didn't show up before in the notebook output without changing any additional code besides the line mentioned. Big one is a function deprecation warning from sklearn https://scikit-learn.org/stable/modules/generated/sklearn.datasets.load_boston.html. Maybe worth making another update or opening a new task to clean those up.

Copy link
Contributor

@vivekmig vivekmig left a comment

Choose a reason for hiding this comment

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

Looks great, thanks so much for fixing this! Just one small nit.

tutorials/House_Prices_Regression_Interpret.ipynb Outdated Show resolved Hide resolved
@dbish dbish self-assigned this Aug 22, 2022
@facebook-github-bot
Copy link
Contributor

@dbish has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants