Fixed missing feature names for XGBoost #93
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hey @akhvorov, thank you so much for your PR! The fix is really great 👍 May I please ask you to add a unit test to make sure that the issue is gone? Otherwise I can add one myself in the follow-up PR. |
krinart
requested changes
Jul 22, 2019
izeigerman
requested changes
Jul 29, 2019
tests/assemblers/test_xgboost.py
Outdated
| utils.train_model_regression(estimator) | ||
|
|
||
| filename = "tmp.file" | ||
| if os.path.exists(filename): |
There was a problem hiding this comment.
Thanks for adding the test!
May I please ask you to use tmp_dir utility function to avoid polluting user's workspace. Example can be found here:
m2cgen/tests/e2e/executors/base.py
Line 12 in 00abbff
Note that this context manager will clean up the temporary directory automatically so you don't have to do it manually here.
I'd also suggest to use UUID for a filename in place of hardcoded
tmp.file to avoid potential conflicts, since this particular test should not be affected by artifacts which can potentially be produced by other tests.
Great job otherwise 👍
UPD: You can actually use any filename, since the entire tmp directory will be removed once this test is finished.
izeigerman
approved these changes
Jul 30, 2019
|
Thanks for the contribution 👍 |
krinart
approved these changes
Jul 30, 2019
|
Thank you for advices and help! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Now it's not fail if XGBoost model were saved by save without feature names