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

possibility of adding sample_weight to interpret.glassbox.ClassificationTree #534

Closed
busFred opened this issue May 1, 2024 · 6 comments
Closed

Comments

@busFred
Copy link
Contributor

busFred commented May 1, 2024

the fit method of class sklearn.tree.DecisionTreeClassifier has two arguments sample_weight and check_input in addition to the required X and y np.ndarray. I'm wondering if it's possible to just allow additional sample_weight to interpret.glassbox.ClassificationTree.fit method and pass that sample_weight to the underlying sklearn.tree.DecisionTreeClassifier.fit method? I'm wondering if the explaining/visualizing procedure requires samples to have uniform weight?

@paulbkoch
Copy link
Collaborator

Makes sense. I don't believe adding sample_weight would present any complications. check_input would probably be better handled as a kwargs on the fit function, although kwargs on the fit function might violate some scikit-learn rule.

This would be an easy thing to add @busFred. Would you like to do it, or would you prefer I put this on our backlog?

@busFred
Copy link
Contributor Author

busFred commented May 2, 2024

Sure, I can definitely do that. seems to be an easy add. However, I do have questions regarding how to set up development environment. I'm wondering what make install is doing and why does it need administrator privilege? I'm not that familiar with nvm and npm and I don't want to do something that could drastically change my work environment and mess other things up. I can work on that once I get clarification.

(interpret_dev) hungtien@hungtien-T480s-Linux:~/Documents/research/interpret/scripts$ make install
cd /home/hungtien/Documents/research/interpret/shared/vis && npm install

added 1 package, changed 1 package, and audited 1134 packages in 3s

156 packages are looking for funding
  run `npm fund` for details

4 moderate severity vulnerabilities

To address all issues (including breaking changes), run:
  npm audit fix --force

Run `npm audit` for details.
cd /home/hungtien/Documents/research/interpret/shared/vis && npm run build-prod

> @interpretml/[email protected] build-prod
> webpack --mode production

asset interpret-inline.js 4.04 MiB [compared for emit] [minimized] [big] (name: main) 1 related asset
orphan modules 339 KiB [orphan] 59 modules
runtime modules 2.62 KiB 8 modules
cacheable modules 9.62 MiB
  modules by path ./node_modules/ 9.32 MiB 26 modules
  modules by path ./src/ 307 KiB
    ./src/index.js + 54 modules 305 KiB [built] [code generated]
    ./node_modules/css-loader/dist/cjs.js!./node_modules/sass-loader/dist/cjs.js!./src/styles.scss 1.38 KiB [built] [code generated]

WARNING in asset size limit: The following asset(s) exceed the recommended size limit (244 KiB).
This can impact web performance.
Assets: 
  interpret-inline.js (4.04 MiB)

WARNING in entrypoint size limit: The following entrypoint(s) combined asset size exceeds the recommended limit (244 KiB). This can impact web performance.
Entrypoints:
  main (4.04 MiB)
      interpret-inline.js


WARNING in webpack performance recommendations: 
You can limit the size of your bundles by using import() or require.ensure to lazy load some parts of your application.
For more info visit https://webpack.js.org/guides/code-splitting/

webpack 5.91.0 compiled with 3 warnings in 81571 ms
mkdir -p "/home/hungtien/Documents/research/interpret/python/interpret-core/interpret/root/bld/lib"
cd /home/hungtien/Documents/research/interpret/shared/vis/dist && cp "interpret-inline.js" "/home/hungtien/Documents/research/interpret/python/interpret-core/interpret/root/bld/lib/"
cd /home/hungtien/Documents/research/interpret/shared/vis/dist && cp "interpret-inline.js.LICENSE.txt" "/home/hungtien/Documents/research/interpret/python/interpret-core/interpret/root/bld/lib/"
/home/hungtien/Documents/research/interpret/build.sh
make: /home/hungtien/Documents/research/interpret/build.sh: Permission denied
make: *** [Makefile:92: build-native] Error 127

@paulbkoch
Copy link
Collaborator

It appears the JavaScript for the visualizations were built, but not the shared library with the C++ code. Presumably because of the permissions issue you mentioned.

Can you try running (without sudo):
/home/hungtien/Documents/research/interpret/build.sh

@paulbkoch
Copy link
Collaborator

If that doesn't work @busFred, you shouldn't need the C++ shared library to modify interpret.glassbox.ClassificationTree since the C++ is only used for EBMs. Many of the tests would fail if you ran pytest locally, but if you submit a PR all the tests will be run.

@busFred
Copy link
Contributor Author

busFred commented May 2, 2024

i added it to #537. let me know if there is anything that you would like me to change

@paulbkoch
Copy link
Collaborator

Thanks @busFred! The PR is merged, so closing this issue.

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

No branches or pull requests

2 participants