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

See Explorer output code within a tab #1156

Merged
merged 72 commits into from Oct 12, 2023
Merged

See Explorer output code within a tab #1156

merged 72 commits into from Oct 12, 2023

Conversation

ahuang11
Copy link
Collaborator

image

@ahuang11 ahuang11 changed the title See code as a tab. See Explorer output code within a tab Sep 26, 2023
Copy link
Member

@maximlt maximlt left a 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 just code/plot_repr/code_repr/... to avoid breaking current users of plot_code)
  • plot_code doesn't infer the variable name, it defaults to df 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]

hvplot/ui.py Outdated Show resolved Hide resolved
@maximlt
Copy link
Member

maximlt commented Sep 27, 2023

Also @ahuang11 you could please add to the 0.9.0 milestone the PRs you need in that release?

@ahuang11 ahuang11 added this to the 0.9.0 milestone Sep 27, 2023
hvplot/ui.py Outdated Show resolved Hide resolved
hvplot/ui.py Outdated Show resolved Hide resolved
@maximlt maximlt requested a review from Hoxbro October 2, 2023 08:22
Copy link
Member

@maximlt maximlt left a 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```"""
Copy link
Member

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.

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 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.

Copy link
Member

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?

Copy link
Collaborator Author

@ahuang11 ahuang11 Oct 9, 2023

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.

hvplot/ui.py Outdated Show resolved Hide resolved
@ahuang11
Copy link
Collaborator Author

ahuang11 commented Oct 8, 2023

Initially I wanted to merge _build_code_snippet and plot_code, but I believe you suggested:

there's an issue somewhere to turn plot_code into a Parameter (that'd maybe just code/plot_repr/code_repr/... to avoid breaking current users of plot_code)

I agree we can simply deprecate plot_code.

Co-authored-by: Maxime Liquet <35924738+maximlt@users.noreply.github.com>
@maximlt
Copy link
Member

maximlt commented Oct 9, 2023

Sorry I wasn't clear enough, I meant that we couldn't just remove plot_code or turn it into a Parameter. I believe though it's fine changing its code, I doubt there's someone doing any kind of parsing on its output.

hvplot/ui.py Outdated Show resolved Hide resolved
@ahuang11
Copy link
Collaborator Author

ahuang11 commented Oct 9, 2023

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.

@ahuang11 ahuang11 requested a review from maximlt October 9, 2023 19:38
@ahuang11
Copy link
Collaborator Author

ahuang11 commented Oct 9, 2023

Tests failing seem unrelated to this PR:

File /usr/share/miniconda3/envs/test-environment/lib/python3.8/site-packages/geoviews/data/geom_dict.py:327, in geom_from_dict(geom, xdim, ydim, single_type, multi_type)
    325 split_holes = geom.pop('holes', None)
    326 if split_holes is not None and len(split_holes) != len(split_geoms):
--> 327     raise DataError('Polygons with holes containing multi-geometries '
    328                     'must declare a list of holes for each geometry.')
    330 if single_type is Point:
    331     if len(splits) > 1 or any(len(g) > 1 for g in split_geoms):

DataError: Polygons with holes containing multi-geometries must declare a list of holes for each geometry.

A new version of maybe geopandas / shapely may have broke something; works locally.
image

rasm = xr.tutorial.open_dataset('rasm').load()



rasm.hvplot.contourf(
    'xc', 'yc', crs=ccrs.PlateCarree(), projection=ccrs.PlateCarree(),
    ylim=(0, 90), frame_width=800, cmap='viridis', levels=10,
    coastline=True
)

Base automatically changed from hvplot_gridded_explorer to main October 12, 2023 13:18
@maximlt
Copy link
Member

maximlt commented Oct 12, 2023

  • I merged main into this branch
  • I decided against having import.<backend> displayed in the code
    • I believe it's not always going to be very useful
    • We've had issues (resolved now I think) with handling multiple imports
    • So, I wasn't sure about this feature, and as it's adding is easier than removing, I prefer postponing adding this feature in a next release, if we decide it's the right thing to do

@maximlt maximlt merged commit 75b6585 into main Oct 12, 2023
7 checks passed
@maximlt maximlt deleted the explorer_code branch October 12, 2023 16:17
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

4 participants