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

Fix plot ensemble model on Windows #3259

Merged
merged 4 commits into from
Jun 7, 2023

Conversation

yinweisu
Copy link
Collaborator

@yinweisu yinweisu commented Jun 1, 2023

Issue #, if available:
#1065

Description of changes:

  • path attributes already being stored in the node is causing some weird issue to pygraphviz on Windows likely because it somehow treat \\ separator on Windows as escape character.
  • This PR removes all unneeded attributes from the nodes

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@yinweisu yinweisu requested a review from Innixma June 1, 2023 22:26
@@ -111,6 +111,7 @@ def test_advanced_functionality():
savedir_predictor_original = savedir + 'predictor/'
predictor: TabularPredictor = TabularPredictor(label=label, path=savedir_predictor_original).fit(train_data)
leaderboard = predictor.leaderboard(data=test_data)
predictor.plot_ensemble_model()
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to install pygraphviz for unit tests to work, it isn't installed by default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see. it might be tricky to install this in our CI env as it requires some C++ library. I'll take a look

@@ -219,6 +220,7 @@ def test_advanced_functionality():

assert(predictor.get_model_full_dict() == dict())
predictor.refit_full()
predictor.plot_ensemble_model()
Copy link
Contributor

Choose a reason for hiding this comment

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

can check that the output file exists on disk, probably that is good enough for testing purposes

@Innixma Innixma added this to the 0.8 Release milestone Jun 6, 2023
@github-actions
Copy link

github-actions bot commented Jun 7, 2023

Job PR-3259-2efc65f is done.
Docs are uploaded to http://autogluon-staging.s3-website-us-west-2.amazonaws.com/PR-3259/2efc65f/index.html

Copy link
Contributor

@Innixma Innixma left a comment

Choose a reason for hiding this comment

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

LGTM assuming the tests pass on Windows

@yinweisu
Copy link
Collaborator Author

yinweisu commented Jun 7, 2023

Have tested on Windows. Will merge now

@yinweisu yinweisu merged commit ec428c5 into autogluon:master Jun 7, 2023
28 checks passed
@yinweisu yinweisu deleted the fix_plot_ensemble branch June 7, 2023 17:21
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.

None yet

2 participants