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

Facetgrid (multiplots) #136

Merged
merged 32 commits into from
Feb 15, 2024
Merged

Facetgrid (multiplots) #136

merged 32 commits into from
Feb 15, 2024

Conversation

sarahclaude
Copy link
Collaborator

I have done some work on gridmap and scattermap to produce facetgrids (multiplots) with the option;
plot_kw = {'col': 'coordinate', 'row': "coordinate"}.

Gridmap seems to be working fine and I am still working on scattermap (the marker sizes can't be send directly to xr.plot.scatter since the results given back are weird).

Once done I will start hatchmap. I think the options to add multiple xr.Datasets/DataArray via a dictionary should be removed since it will be confusing if passed with plot_kw = {'col': 'coordinate'}. This option is also absent in the two other maps options.

Copy link
Contributor

@juliettelavoie juliettelavoie left a comment

Choose a reason for hiding this comment

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

as-tu réussi à faire une gridmap et scattermap ensemble sur une figure? Je pense que ça va être plus compliquer qu'on pensait.. https://stackoverflow.com/questions/44158276/combine-different-seaborn-facet-grids-into-single-plot

figanos/matplotlib/plot.py Outdated Show resolved Hide resolved
figanos/matplotlib/plot.py Outdated Show resolved Hide resolved
figanos/matplotlib/plot.py Show resolved Hide resolved
figanos/matplotlib/plot.py Outdated Show resolved Hide resolved
figanos/matplotlib/plot.py Outdated Show resolved Hide resolved
@juliettelavoie
Copy link
Contributor

Maybe I would put the y label of rows in the left side of the facetgrid instead of the rigth, next to the colorbar.. Or I would put the colorbar at the bottom. It just feels a bit crowded to me.

@vindelico
Copy link
Contributor

vindelico commented Nov 23, 2023

as-tu réussi à faire une gridmap et scattermap ensemble sur une figure? Je pense que ça va être plus compliquer qu'on pensait.. https://stackoverflow.com/questions/44158276/combine-different-seaborn-facet-grids-into-single-plot

The link suggests you are asking about having a facetgrid with gridmap(s) and scattermap(s) alongside each other, i.e. different tiles of the facetgrid have either gridmap(s) or scattermap(s).
A different way of combining them is my use case for stacking facetgrids of gridmaps and of scattermaps, so that the scattermaps are superposed over the gridmaps, in other words: dots on maps. I will look into that.

@sarahclaude
Copy link
Collaborator Author

sarahclaude commented Nov 29, 2023

as-tu réussi à faire une gridmap et scattermap ensemble sur une figure? Je pense que ça va être plus compliquer qu'on pensait.. https://stackoverflow.com/questions/44158276/combine-different-seaborn-facet-grids-into-single-plot

The link suggests you are asking about having a facetgrid with gridmap(s) and scattermap(s) alongside each other, i.e. different tiles of the facetgrid have either gridmap(s) or scattermap(s). A different way of combining them is my use case for stacking facetgrids of gridmaps and of scattermaps, so that the scattermaps are superposed over the gridmaps, in other words: dots on maps. I will look into that.

I think we should try these functions instead since xarray creates its own facetgrid object with its own methods : https://docs.xarray.dev/en/stable/generated/xarray.plot.FacetGrid.html

After trying these functions with figanos and xarray.plot, I am not sure we can make them work the way we want. I only get mutiple facetgrids and they are not put on top of each other.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@juliettelavoie
Copy link
Contributor

Re: 2 facetgrid together. I haven't found a way to combine facetgrid yet. But it it possible to modify individually each subplot like so:

import numpy as np
air = xr.tutorial.open_dataset("air_temperature").air
ax=air.isel(lon=10, lat=[19, 21, 22]).plot.line(x="time", col='lat')
ax.axes[0][0].plot([np.datetime64('2014-01-02T00:00:00.000000000')],[295], '.')

Hence it would be possible (though not very elegant) to add a scatter map by hand on top of a gridmap.

@sarahclaude
Copy link
Collaborator Author

Should we add arguments similar to vmin & vmax for the marker size in scattermap? If not there's no way to use the same values for multiple plots in a loop (it would only work when using 'col' or 'row' in plot_kw)

@vindelico
Copy link
Contributor

vindelico commented Dec 11, 2023

as-tu réussi à faire une gridmap et scattermap ensemble sur une figure? Je pense que ça va être plus compliquer qu'on pensait.. https://stackoverflow.com/questions/44158276/combine-different-seaborn-facet-grids-into-single-plot

The link suggests you are asking about having a facetgrid with gridmap(s) and scattermap(s) alongside each other, i.e. different tiles of the facetgrid have either gridmap(s) or scattermap(s). A different way of combining them is my use case for stacking facetgrids of gridmaps and of scattermaps, so that the scattermaps are superposed over the gridmaps, in other words: dots on maps. I will look into that.

I think we should try these functions instead since xarray creates its own facetgrid object with its own methods : https://docs.xarray.dev/en/stable/generated/xarray.plot.FacetGrid.html

After trying these functions with figanos and xarray.plot, I am not sure we can make them work the way we want. I only get mutiple facetgrids and they are not put on top of each other.

I looked into xarray.plot.FacetGrid.map* methods (and the corresponding methods in seaborn.FacetGrid for pandas.DataFrame). It seems that the mapping can only use data from the underlying dataset/dataframe from a first use of row/col/col_wrap args, generating the facetgrid. Correct me if I'm wrong but I believe overlaying facetgrids would require to add another dataset as an attribute of the facetgrid object, and modifying the map* methods, particularly when the structure is different, which is so with the "dots on maps" use case and our differing data structure for grids vs station data which can not be merged into one xarray.Dataset.

That said, iterating over the facets (axs) in the facetgrid is not so un-elegant at this point.

if facetgrid:
if "suptitle" in attr_dict:
suptitle = get_attributes(attr_dict["suptitle"], xr_obj)
facetgrid.fig.suptitle(suptitle, y=1.05)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like placing the title above the plot with y>1 only works in notebooks:
https://stackoverflow.com/questions/55767312/how-to-position-suptitle#comment98213058_55768955
I have successfully used it in notebooks, but it fails for me in SciView/PyCharm.

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 can remove the y=1.05, but I have noticed that playing with the figsize also helps with suptitle

figanos/matplotlib/plot.py Outdated Show resolved Hide resolved
figanos/matplotlib/plot.py Outdated Show resolved Hide resolved
docs/notebooks/figanos_multiplots.ipynb Show resolved Hide resolved
@sarahclaude
Copy link
Collaborator Author

Gridmap, hatchmap and scattermap should now be working with facetgrids

@sarahclaude sarahclaude marked this pull request as ready for review January 25, 2024 19:42
@Zeitsperre
Copy link
Contributor

FYI, this is the sole offending warning on ReadTheDocs:

checking consistency... /home/docs/checkouts/readthedocs.org/user_builds/figanos/checkouts/136/docs/notebooks/figanos_multiplots.ipynb: WARNING: document isn't included in any toctree

@@ -0,0 +1,344 @@
{
Copy link
Contributor

@juliettelavoie juliettelavoie Jan 29, 2024

Choose a reason for hiding this comment

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

comme il n'y a pas d'exemple ici, je l'enleverrais ou j'écrirais "Section coming soon".


Reply via ReviewNB

@@ -0,0 +1,344 @@
{
Copy link
Contributor

@juliettelavoie juliettelavoie Jan 29, 2024

Choose a reason for hiding this comment

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

juste une question d'esthétique. J'ajouterais        legend_kw={'ncol':4,'bbox_to_anchor':(0.15, 0.05)


Reply via ReviewNB

@@ -0,0 +1,344 @@
{
Copy link
Contributor

@juliettelavoie juliettelavoie Jan 29, 2024

Choose a reason for hiding this comment

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

avec xclim 0.47, cette cellule brise parce que change_significance a une nouvelle signature.

https://xclim.readthedocs.io/en/stable/changes.html

change_significance va m[eme disparaitre à 0.49. but this can be fixed in another PR.


Reply via ReviewNB

Copy link
Contributor

@juliettelavoie juliettelavoie left a comment

Choose a reason for hiding this comment

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

great job!

@juliettelavoie
Copy link
Contributor

@Zeitsperre is it possible that something you changed recently put a lower timeout or memory restriction on the RTD build ?

@Zeitsperre
Copy link
Contributor

@juliettelavoie No, there's nothing significantly impacting to the docs that was changed from my end. No additional commands are being called, no new dependencies. The only changes I added are either commented out or they have no impact (unused locale-related changes)

@juliettelavoie
Copy link
Contributor

Le probleme est dans la cellule hatchmap. C'est trop lourd de loader les 3 modèles. Je suggère de faire un exemple sans change_significance, genre juste un hatch si tas>30.

@sarahclaude
Copy link
Collaborator Author

Added facetgrids for timeseries (should be the last added function on this PR)

@@ -0,0 +1,371 @@
{
Copy link
Contributor

@juliettelavoie juliettelavoie Feb 15, 2024

Choose a reason for hiding this comment

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

I might change this to just " Multiple Plots"


Reply via ReviewNB

Copy link
Contributor

@juliettelavoie juliettelavoie left a comment

Choose a reason for hiding this comment

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

I think it is ready to merge! so much work in here! nice job!

@sarahclaude sarahclaude merged commit 3084e8e into main Feb 15, 2024
17 checks passed
@sarahclaude sarahclaude deleted the facetgrids branch February 15, 2024 22:36
@sarahclaude sarahclaude mentioned this pull request Feb 16, 2024
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.

4 participants