Issue241 rcm param and ducktyping for models #248
Conversation
|
@bdewilde hey, as promised, sent over a pull request about the two enhancement mentioned. Cheers, |
|
Hey @zf109 , thanks for the PR! I've been busy packing / moving / unpacking my life the past couple weeks, so please bear with me a little longer as I get back up to speed. Thanks again, will review this soon. 👍 |
| @@ -138,6 +141,10 @@ def draw_termite_plot( | |||
| raise ValueError(msg) | |||
| highlight_colors = {hc: COLOR_PAIRS[i] for i, hc in enumerate(highlight_cols)} | |||
|
|
|||
| if rc_params: | |||
There was a problem hiding this comment.
This block overwrites parts of the global RC_PARAMS with whatever is passed in the function call, but it would be safer to modify a local version of the params. Could you do something like
_rc_params = RC_PARAMS.copy()
if rc_params:
_rc_params.update(rc_params)
with plt.rc_context(_rc_params):
...
instead?
There was a problem hiding this comment.
@bdewilde yeah sure, it's better indeed :)
| @@ -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, | |||
textacy/tm/topic_model.py
Outdated
| @@ -392,6 +398,8 @@ def termite_plot( | |||
| the default ("seriation") groups similar terms together, which | |||
| facilitates cross-topic assessment | |||
| save (str): 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.
same comments as on the other docstring
textacy/tm/topic_model.py
Outdated
| def __init__(self, model, n_topics=10, **kwargs): | ||
| if isinstance(model, (NMF, LatentDirichletAllocation, TruncatedSVD)): | ||
| self.model = model | ||
| elif all([hasattr(model, required_attr) for required_attr in self._required_trained_model_attr]): |
There was a problem hiding this comment.
nit: you can drop the brackets on the list ([ and ])
| @@ -103,9 +104,13 @@ class TopicModel(object): | |||
| - http://scikit-learn.org/stable/modules/generated/sklearn.decomposition.TruncatedSVD.html | |||
| """ | |||
|
|
|||
| _required_trained_model_attr = {"transform", "components_", "n_topics"} | |||
There was a problem hiding this comment.
Scanning through the code, it looks like models also need a .fit() method and n_components attribute. Is that doable?
There was a problem hiding this comment.
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.
| if rc_params: | ||
| for key in rc_params: | ||
| RC_PARAMS[key] = rc_params[key] | ||
| _rc_params.update(rc_params) | ||
|
|
||
| with plt.rc_context(RC_PARAMS): |
There was a problem hiding this comment.
Almost there! You forgot to change this line: RC_PARAMS => _rc_params
|
Actually, I'm just going to merge this in and make that minor hotfix in |
|
Welp, looks like it actually went into |
|
@bdewilde ha, great, sorry was a bit busy last few weeks and on holiday then so didn't see your comments... thanks a lot for merging it :) |
Description
Solving Issue 241, please see details there.
Motivation and Context
A feature request: allows duck typing for instantiate
TopicModelclass and allow passingrc_paramsto thedrew_termite_plotfunction so that the termite plot can be more flexible.How Has This Been Tested?
rc_paramsand see if the plot changed as expected.Types of changes
Checklist: