-
Notifications
You must be signed in to change notification settings - Fork 248
Issue241 rcm param and ducktyping for models #248
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
Changes from all commits
a76230f
a59255e
62b23e3
d0e45ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,6 +71,7 @@ def draw_termite_plot( | |
highlight_cols=None, | ||
highlight_colors=None, | ||
save=False, | ||
rc_params=None, | ||
): | ||
""" | ||
Make a "termite" plot, typically used for assessing topic models with a tabular | ||
|
@@ -87,6 +88,8 @@ def draw_termite_plot( | |
of (light/dark) matplotlib-friendly colors used to highlight a single | ||
column; if not specified (default), a good set of 6 pairs are used | ||
save (str, optional): give the full /path/to/fname on disk to save figure | ||
rc_params (dict, optional): allow passing parameters to rc_context in matplotlib.plyplot, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah.. missed this one... sorry |
||
details in https://matplotlib.org/3.1.0/api/_as_gen/matplotlib.pyplot.rc_context.html | ||
|
||
Returns: | ||
:obj:`matplotlib.axes.Axes.axis`: Axis on which termite plot is plotted. | ||
|
@@ -138,6 +141,10 @@ def draw_termite_plot( | |
raise ValueError(msg) | ||
highlight_colors = {hc: COLOR_PAIRS[i] for i, hc in enumerate(highlight_cols)} | ||
|
||
_rc_params = RC_PARAMS.copy() | ||
if rc_params: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This block overwrites parts of the global
instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bdewilde yeah sure, it's better indeed :) |
||
_rc_params.update(rc_params) | ||
|
||
with plt.rc_context(RC_PARAMS): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Almost there! You forgot to change this line: |
||
fig, ax = plt.subplots(figsize=(pow(n_cols, 0.8), pow(n_rows, 0.66))) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scanning through the code, it looks like models also need a
.fit()
method andn_components
attribute. Is that doable?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree with the
n_components
, but would be good to leave.fit
method a bit optional, essentially it allows to initialize with a trained model, so we don't need to call the training method. But up to you I guess.