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

[PNE-6511] Expose feature effects generation. #983

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

anoto-moniz
Copy link
Collaborator

At present, the backend will automatically generate feature effects upon successful training. However, that means old models lack them. Additionally, there may be circumstances where regenerating the values may be desired.

The exposed endpoint will begin an asynchronous job which will generate new Shapley values, and replace the old ones.

PR Type:

  • Breaking change (fix or feature that would cause existing functionality to change)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Maintenance (non-breaking change to assist developers)

Adherence to team decisions

  • I have added tests for 100% coverage
  • I have written Numpy-style docstrings for every method and class.
  • I have communicated the downstream consequences of the PR to others.
  • I have bumped the version in __version__.py

At present, the backend will automatically generate feature effects upon
successful training. However, that means old models lack them.
Additionally, there may be circumstances where regenerating the values
may be desired.

The exposed endpoint will begin an asynchronous job which will generate
new Shapley values, and replace the old ones.
@anoto-moniz anoto-moniz marked this pull request as ready for review January 8, 2025 18:39
@anoto-moniz anoto-moniz requested a review from a team as a code owner January 8, 2025 18:39
Comment on lines +54 to +56
shapley = data.get("result")
if not shapley:
return data
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There was a bug in the existing code which caused it to blow up if there was no result field, which would happen if either Shapley hadn't been run, or if the generation was in progress.

Comment on lines +204 to +205
self.session.put_resource(path, {}, version=self._api_version)
return self.get(uid, version=version)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The FeatureEffects object returned by the generate call will always be in progress and have no results yet. Additionally, as currently constructed, it doesn't provide an easy way to get the updated status: you'd have to retrieve the corresponding predictor, then call .feature_effects on it. So instead, this does the get automatically.

Copy link
Collaborator

@kroenlein kroenlein left a comment

Choose a reason for hiding this comment

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

Something seems aesthetically weird here, but all lines up and presumably passes tests. Will you be adding a follow-on PR to devkit for e2es?

@anoto-moniz anoto-moniz merged commit d4a2e39 into main Jan 9, 2025
17 checks passed
@anoto-moniz anoto-moniz deleted the feature/pne-6511-shapley-generation branch January 9, 2025 15:42
anoto-moniz added a commit that referenced this pull request Jan 16, 2025
…511-shapley-generation"

This reverts commit d4a2e39, reversing
changes made to 556ce5f.
anoto-moniz added a commit that referenced this pull request Jan 16, 2025
…511-shapley-generation"

This reverts commit d4a2e39, reversing
changes made to 556ce5f.
anoto-moniz added a commit that referenced this pull request Jan 16, 2025
Revert "Merge pull request #983 from CitrineInformatics/feature/pne-6…
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