Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Model Exploration step 3 seems to be broken #60

Closed
riley-harper opened this issue Nov 30, 2022 · 3 comments · Fixed by #101
Closed

Model Exploration step 3 seems to be broken #60

riley-harper opened this issue Nov 30, 2022 · 3 comments · Fixed by #101
Labels
component: model exploration type: bug Something isn't working

Comments

@riley-harper
Copy link
Contributor

This step has been breaking for me recently, so I took a look at it. It seems to me that model_exploration.link_step_get_feature_importances is not reading in its chosen model correctly. It expects step 2 (link_step_train_test_models) to serialize the model to a particular path. But I believe step 2 instead saves the model to a dictionary on the LinkRun.

This causes step 3 to error out each time it looks for the chosen model because it can't find it. We don't have any tests that cover this module, so adding some tests that run all of the steps for Model Exploration in a row might be very helpful.
@riley-harper riley-harper added the type: bug Something isn't working label Nov 30, 2022
@riley-harper
Copy link
Contributor Author

See also #29.

@riley-harper
Copy link
Contributor Author

And #30 is also related.

To fix these related issues, I'm going to move much of the functionality for Model Exploration's step 3 to a new training step 3. This step will save model metadata including feature importances. The Model Exploration step 3 will be deprecated and eventually removed.

riley-harper added a commit that referenced this issue Oct 18, 2023
@riley-harper
[#60] Copy Model Exploration step 3 to a new Training step 3
d22024f 
This is not hooked up and polished yet. The new step will save metadata
about the model trained in the previous step for inspection and debugging.
riley-harper added a commit that referenced this issue Oct 18, 2023
@riley-harper
[#60] Hook up the new save model metadata training step 3
8622b44
riley-harper added a commit that referenced this issue Oct 18, 2023
@riley-harper
[#60] Skip training step 3 unless training.feature_importances is set…
d26daa4 
… to true in the config file
riley-harper added a commit that referenced this issue Oct 18, 2023
@riley-harper
[#60] Add some tests for training step 3
2f3144e 
One of these tests is failing because LinkStepSaveModelMetadata is erroring out
when it's not skipped.
riley-harper added a commit that referenced this issue Oct 18, 2023
@riley-harper
[#60] Remove some now-redundant if statements from training step 3
1ccc9f7
riley-harper added a commit that referenced this issue Oct 18, 2023
@riley-harper
[#60] Read the model from the trained_models dict
0aedb7e 
It is not serialized to the filesystem anymore. This does mean that it
doesn't persist between runs of hlink, so I added a note to that effect.
riley-harper added a commit that referenced this issue Oct 18, 2023
@riley-harper
[#60] Implement training step 3 to get feature importances
d85d28f 
This seems to be working nicely. There's still a little bit to iron out about
feature importances vs. coefficients for different models.
riley-harper added a commit that referenced this issue Oct 19, 2023
@riley-harper
[#60] Remove an unused private method from matching's score step
3906b58 
We've done what the TODO comment here says and created a step in training that
does this for us. So we can get rid of this method to keep things organized.
riley-harper added a commit that referenced this issue Oct 19, 2023
@riley-harper
[#60] Remove Model Exploration's get feature importances step
e6ab2d1 
We could deprecate this and treat it as a breaking change, but I see two reasons
to just go ahead and remove this as a bug fix:

1. This step is undocumented.
2. This step is really buggy and does not work at all. Users really should not be using
   it. Now training step 3 does what this step was supposed to do.
riley-harper added a commit that referenced this issue Oct 23, 2023
@riley-harper
[#60] Remove an unused import
53be2b9
riley-harper added a commit that referenced this issue Oct 23, 2023
@riley-harper
[#60] Improve documentation for training save model metadata step
ccf60c4 
Add a doc comment and remove some unhelpful code comments.
riley-harper added a commit that referenced this issue Oct 23, 2023
@riley-harper
[#60] Rename training_feature_importances column to coefficient_or_im…
8edbd88 
…portance
riley-harper added a commit that referenced this issue Oct 23, 2023
@riley-harper
[#60] Sort training_feature_importances on the new column name
47347b4
riley-harper added a commit that referenced this issue Oct 23, 2023
@riley-harper
[#60] Update documentation for the new training step 3
d793638 
I've documented that this step is available only for Training, not for Household
Training at the moment. We can easily add this functionality to Household Training
as well if it would be useful.
riley-harper added a commit that referenced this issue Oct 23, 2023
@riley-harper
[#60] Fix a bug that appeared in two training step 3 tests
d59f1ea 
We need to access training.feature_importances, not a top-level feature_importances
attribute.
@riley-harper
Copy link
Contributor Author

After taking a closer look at the old Model Exploration step 3, it was clearly a rough draft and not production ready. There were numerous bugs, and it was entirely undocumented. So I'm going to treat this as a bug fix and remove Model Exploration step 3 without deprecating it. Users that are looking for similar functionality should use Training step 3 instead now.

@riley-harper riley-harper mentioned this issue Oct 23, 2023
Merged
# for free to join this conversation on GitHub. Already have an account? # to comment
Assignees
No one assigned
Labels
component: model exploration type: bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Implement a new Training step that replaces Model Exploration step 3 ipums/hlink
1 participant
@riley-harper