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

Bug fix: Include forward_args in self.model function calls in intergrated_gradients.py #525

Merged
merged 7 commits into from
Nov 16, 2021

Conversation

sanjass
Copy link
Contributor

@sanjass sanjass commented Nov 9, 2021

Bug report + fix: forward_kwargs not used in all self.model calls in integrated_gradients.py
#524

Bug report + fix: forward_kwargs not used in all self.model calls in integrated_gradients
SeldonIO#524
@sanjass sanjass changed the title Update integrated_gradients.py Include forward_args in self.model function calls in intergrated_gradients.py Nov 9, 2021
@sanjass sanjass changed the title Include forward_args in self.model function calls in intergrated_gradients.py Bug fix: Include forward_args in self.model function calls in intergrated_gradients.py Nov 9, 2021
@jklaise
Copy link
Contributor

jklaise commented Nov 10, 2021

@sanjass the tests are failing because we also pass in forward_kwargs when it is None. We would need to add the same logic to these calls as here:

if forward_kwargs is None:
preds = model(x)
else:
preds = model(x, **forward_kwargs)

Given the repetition I think it would be good to split this out into a small utility function (or actually, would be good to check if we could just substitute None for the empty dictionary {} and it might run without having to do this check).

replaced forward_kwargs with {} when it's None
@sanjass
Copy link
Contributor Author

sanjass commented Nov 10, 2021

I see, I pushed a change as follows:

if forward_kwargs is None:
    forward_kwargs = {}

and replaced If forward_kwargs is not None-> if forward_kwargs
also removed if forward_kwargs is None and passed **forward_kwargs as before

Didn't get to run the checks though

replacing forward_kwargs None with {} in all functions
@jklaise
Copy link
Contributor

jklaise commented Nov 10, 2021

Thanks! Changes look good, I will rerun the IG notebooks (especially the transformers one) before merging.

added missing check " if forward_kwargs is None: forward_kwargs = {}"
@codecov
Copy link

codecov bot commented Nov 11, 2021

Codecov Report

Merging #525 (ea3f40a) into master (6100135) will decrease coverage by 0.02%.
The diff coverage is 81.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #525      +/-   ##
==========================================
- Coverage   82.34%   82.32%   -0.03%     
==========================================
  Files          76       76              
  Lines       10334    10350      +16     
==========================================
+ Hits         8510     8521      +11     
- Misses       1824     1829       +5     
Impacted Files Coverage Δ
alibi/explainers/integrated_gradients.py 88.91% <81.25%> (-0.89%) ⬇️

@jklaise
Copy link
Contributor

jklaise commented Nov 11, 2021

@sanjass just a suggestion, if you also do pip install -r requirements/dev.txt and run flake8 alibi from the repo source you can see all the linting errors and fix them locally :)

@sanjass
Copy link
Contributor Author

sanjass commented Nov 11, 2021

Yeah, did something like that but my previous fix introduced a different flake8 error and I didn't double check lol. Also I'm editing in the github editor directly. Anyways hopefully this finally works

@RobertSamoilescu
Copy link
Collaborator

RobertSamoilescu commented Nov 15, 2021

@sanjass, we will be working on issue #527 before merging this PR.

@jklaise jklaise merged commit 84b17aa into SeldonIO:master Nov 16, 2021
@jklaise
Copy link
Contributor

jklaise commented Nov 16, 2021

@sanjass thank you for your contribution! And thanks for the extra review @RobertSamoilescu!

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.

3 participants