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.

Sign up for GitHub

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

Jump to bottom

Discussion: model_parts n_sample and model_profile N #175

Closed
hbaniecki opened this issue Mar 30, 2020 · 3 comments · Fixed by #201
Closed

Discussion: model_parts n_sample and model_profile N #175

hbaniecki opened this issue Mar 30, 2020 · 3 comments · Fixed by #201
Labels
invalid ❕ This doesn't seem right, potential bug long term 📆 TODO long term Python 🐍 Related to Python R 🐳 Related to R

Comments

@hbaniecki
Copy link
Member

hbaniecki commented Mar 30, 2020 •
edited
Loading

  1. I would argue that n_sample in model_parts should be NULL like in ingredients.

    DALEX/R/model_parts.R

    Line 46 in 1a0c768

    n_sample = 1000) {

  2. Maybe add a message saying that your 20k-row dataset is being cut to only 5%.

  3. Maybe use fraction sampling. If so, consider changing the N in model_profile as well.

  4. Unify n_sample and N into one argument, check if there are other functions using n_sample
@hbaniecki hbaniecki added long term 📆 TODO long term R 🐳 Related to R labels Mar 30, 2020
@pbiecek
Copy link
Member

pbiecek commented Mar 31, 2020

I wonder if there is any significant difference between using 1k vs 20k rows for assessment of feature importance. If not then calculations for 1k will be much faster (and AFAIK now these calculations are repeated B times).

Yes, it is good idea to unify n_sample_ and N if they occur in the same context.

@hbaniecki hbaniecki added the Python 🐍 Related to Python label Apr 8, 2020
hbaniecki pushed a commit that referenced this issue Apr 8, 2020
[python] vi n_sample -> N #175
941c5ef
@hbaniecki hbaniecki mentioned this issue Apr 9, 2020
Merged
pbiecek pushed a commit that referenced this issue Apr 9, 2020
@hbaniecki
[python] dalex v0.1.3 (#177)
8359934 
* [python] add residual function to Explainer

* [python] vi n_sample -> N #175

* [python] add yhat, fix cp&ap axis title

* [python] add residual function to Explainer

* center README images
@hbaniecki hbaniecki removed the Python 🐍 Related to Python label Apr 14, 2020
@pbiecek
Copy link
Member

pbiecek commented Apr 18, 2020

is it fixed? can we close it?

@hbaniecki
Copy link
Member Author

hbaniecki commented Apr 18, 2020 •
edited
Loading

The name of n_samples parameter was changed in Python but remains the same in R.

@hbaniecki hbaniecki added Python 🐍 Related to Python invalid ❕ This doesn't seem right, potential bug labels Apr 21, 2020
This was referenced Apr 21, 2020
Closed
Merged
@kasiapekala kasiapekala mentioned this issue Apr 21, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
invalid ❕ This doesn't seem right, potential bug long term 📆 TODO long term Python 🐍 Related to Python R 🐳 Related to R
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[r] n_sample -> N ModelOriented/DALEX
2 participants
@pbiecek @hbaniecki