-
-
Notifications
You must be signed in to change notification settings - Fork 110
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
See Explorer output code within a tab #1156
Conversation
ahuang11
commented
Sep 26, 2023
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.
Oops failing tests 🙃
Nice feature! Could you please add some tests and documentation?
In your screenshot is it normal there's no kind
in the call to hvplot()
?
Related work/thoughts
- there's an issue somewhere to turn
plot_code
into a Parameter (that'd maybe justcode
/plot_repr
/code_repr
/... to avoid breaking current users ofplot_code
) plot_code
doesn't infer the variable name, it defaults todf
as shown in your example. I found this cool trick that leverages f-strings to obtain the name of a variable, might be worth considering.
my_df = pd.DataFrame()
var_name = f'{my_df=}'.split('=')[0]
Also @ahuang11 you could please add to the 0.9.0 milestone the PRs you need in that release? |
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.
I just gave a quick review before @hoxbro's one.
I'm the one who suggested adding the code
Parameter to the explorer. IIRC, this actually was a suggestion made by @MarcSkovMadsen a while back, I'd be happy to hear from you Marc whether this is what you expected at the time?
If it's alright, I believe the new code
Parameter should be documented. I'm also wondering whether plot_code()
should be deprecated, the only advantage it has is that you can provide a name for the data variable. So now we have .plot_code(var_name='df')
, .code
and _build_code_snippet()
. We could have just code
so not allow users to provide a variable name? The minimum we should do is merge plot_code
and _build_code_snippet
, if we want to expose such a method as public API.
hvplot/ui.py
Outdated
@@ -551,9 +560,22 @@ def _plot(self, *events): | |||
finally: | |||
self._layout.loading = False | |||
|
|||
def _code(self): | |||
self.code = self._build_code_snippet() | |||
self._code_pane.object = f"""```python\nimport hvplot.{self._backend}\n\n{self.code}\n```""" |
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.
I would not include the import, it doesn't bring much in my opinion.
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.
I disagree; it reduces the friction for the user to immediately get started--else, the user needs to manually type import hvplot.pandas
If they do not want the import, they can simply copy up to the line below.
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.
IIRC you have to run the import first in a notebook for the explorer to work. What use case do you have in mind?
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.
This doesn't necessarily have to run in notebook, but if they do, they can copy it to a script or in the future, if they launch the explorer from CLI.
Initially I wanted to merge
I agree we can simply deprecate plot_code. |
Co-authored-by: Maxime Liquet <35924738+maximlt@users.noreply.github.com>
Sorry I wasn't clear enough, I meant that we couldn't just remove |
Okay I refactored this so that _build_code_snippet is now merged into plot_code and also cleaned up unnecessary redundancy with _code. Lastly, I renamed some internal variables for clarity. |
|