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

start of plotting "help" module #258

Merged
merged 23 commits into from
Nov 27, 2023
Merged

Conversation

philippemiron
Copy link
Contributor

@philippemiron philippemiron commented Sep 13, 2023

  • clean up codes
  • wraps the import to matplotlib and cartopy to include an Optional dependencies warning
  • validate that a projection is passed if using a geoaxes
  • add docstring
  • create examples
  • tests

As of right now, the goals are:

@selipot
Copy link
Member

selipot commented Sep 13, 2023

Great start! maybe call the module plothelp or helper instead of help which is a bit too general?

Maybe the two goals could be combined? i.e. the function that returns the LineCollection returns extra lines when crossing the dateline to accommodate? How do we make sure that the assigned colors of the lines for the drifters that cross the dateline are the same?

@milancurcic
Copy link
Member

What do you think about utility? For the function that inserts NaN at dateline, I suggest making a function that inserts NaNs between segments, and let the user segment the dateline. That could fit in the analysis module. I don't quite see the need for a separate module here. I know @selipot suggested it and I didn't say anything, but I don't see it as compelling.

@milancurcic
Copy link
Member

and let the user segment the dateline

We can then have a separate function that segments the dateline specifically, so the desired functionality here would be by chaining the two functions.

@philippemiron
Copy link
Contributor Author

philippemiron commented Sep 13, 2023

I have no preference about the name of the module and/or the location of those functions.

I was going to make the first function to insert nan general enough to deal with any periodic domain, expanding on this:

nan_indices = np.cumsum(cd.analysis.segment(lon, 180, rowsize=cd.analysis.segment(lon, -180)))[:-1]

I can switch it to utility.py for now and we see where we put the functions once ready.

@philippemiron
Copy link
Contributor Author

Great start! maybe call the module plothelp or helper instead of help which is a bit too general?

Maybe the two goals could be combined? i.e. the function that returns the LineCollection returns extra lines when crossing the dateline to accommodate? How do we make sure that the assigned colors of the lines for the drifters that cross the dateline are the same?

If we include nans at the right place, we don't need to split trajectories into 2 segments on either side of the dateline and duplicates colors to properly plot them. And at the same time, it would limit to a single task the function that generates the LineCollection.

@milancurcic milancurcic added the enhancement New feature or request label Sep 15, 2023
@philippemiron
Copy link
Contributor Author

philippemiron commented Sep 18, 2023

Not sure exactly how to proceed now, segment only supports 1-d array... 😭. And I also need a way to update rowsize after adding the nan.

@philippemiron philippemiron marked this pull request as ready for review November 16, 2023 00:55
@philippemiron
Copy link
Contributor Author

@selipot want to test? I guess I might have some time next week during Thanksgiving to play with this.

@philippemiron
Copy link
Contributor Author

philippemiron commented Nov 16, 2023

I'm abandoning the idea of rebasing this branch. Many files have been renamed, deleted and functions moved around, making this very annoying. I might just reopen another PR once we are closer to merging this.

@selipot
Copy link
Member

selipot commented Nov 16, 2023

I'm abandoning the idea of rebasing this branch. Many files have been renamed, deleted and functions moved around, making this very annoying. I might just reopen another PR once we are closer to merging this.

Sorry about that, that happens when a PR is open for too long for a version 0.x software I guess. Do you want me to test before you open another PR or after?

@philippemiron
Copy link
Contributor Author

No worries, the easiest would be just to copy the function.

@philippemiron
Copy link
Contributor Author

One question, how can I test something that requires optional dependencies... 🤔 I guess we would need to have a different environment for the test, which would include all the mandatory and optional packages.

@milancurcic
Copy link
Member

How about installing all dependencies (core and optional) for testing?

@selipot
Copy link
Member

selipot commented Nov 16, 2023

Testing your function. I think there is an issue with your first test lines 132-150. It raises a valueError if ax is a simple plt.Axes which it should as it is written. As an example this does not work:

fig = plt.figure()
ax = fig.add_subplot(1, 1, 1)
plot_ragged(ax, ds.lon, ds.lat, ds.rowsize, colors=np.arange(len(ds.rowsize)));
Screenshot 2023-11-16 at 8 18 03 AM

@selipot
Copy link
Member

selipot commented Nov 16, 2023

The examples need editing which I can do once you submit a new PR.

@philippemiron
Copy link
Contributor Author

Did you try the latest version? You can use this branch, I fix it yesterday.

@selipot
Copy link
Member

selipot commented Nov 16, 2023

I copied the code for the function from the PR branch as you instructed. Again, lines 132-150 don't make sense to me or rather the error I get makes sense. Why would you test for ax to have the attribute coastline if it is only plt.Axes?

if isinstance(ax, plt.Axes) and hasattr(
         ax, "coastlines"
     )
...

@philippemiron
Copy link
Contributor Author

philippemiron commented Nov 16, 2023

If it has attributes coastline, it is a geoAxe and we have to also try to load cartopy. This way, the function can be used with matplotlib only.

The else condition should be one (or two) level(s) further aligned with the if isinstance(geoAxe). I'm on the plane but can fix in ~2h.

@selipot
Copy link
Member

selipot commented Nov 16, 2023

Yes but the exception to your if statement is an error so that you get an error still if the axis is simply plt.Axes. The function works for me if I comment that if statement.

@philippemiron
Copy link
Contributor Author

@selipot fix now.

@selipot
Copy link
Member

selipot commented Nov 21, 2023

What's next with this @philippemiron ?

@philippemiron
Copy link
Contributor Author

Did you have chance to give it a try?

@selipot
Copy link
Member

selipot commented Nov 21, 2023

Yes! Works for me. But did not test with cartopy.

Copy link
Member

@milancurcic milancurcic left a comment

Choose a reason for hiding this comment

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

This looks good to go to me. I installed it, plotted some stuff, looks good. Tests pass after installing matplotlib and cartopy.

utility as a module name is quite generic. Should we call this plotting?

clouddrift/utility.py Outdated Show resolved Hide resolved
the trajectories are splitted into segments and each segment is colored according
to the corresponding color value. If colors is the same shape as rowsize, the
trajectories are uniformly colored according to the corresponding color value.
*args : tuple
Copy link
Member

Choose a reason for hiding this comment

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

**kwargs are below. LGTM.

@@ -0,0 +1,149 @@
import cartopy.crs as ccrs
from clouddrift.utility import plot_ragged
import matplotlib.pyplot as plt
Copy link
Member

Choose a reason for hiding this comment

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

Is there a mechanism to skip these tests if optional dependencies are not installed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I assumed that it was installed in the ci/cd, but I see that this wouldn't work if you don't have those packages locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you try again with the latest modification?

milancurcic and others added 4 commits November 22, 2023 09:50
@milancurcic
Copy link
Member

I forgot one important thing: we should add a note to the installation instructions (in README.md and docs/install.rst) about installing optional dependencies.

@milancurcic
Copy link
Member

I think we discussed and established that matplotlib-core is already implicitly installed by depending on xarray, correct? In that case, I think only cartopy should be an optional dependency.

@philippemiron
Copy link
Contributor Author

If I installed the environment I don't get matplotlib.

@philippemiron
Copy link
Contributor Author

I think we discussed and established that matplotlib-core is already implicitly installed by depending on xarray, correct? In that case, I think only cartopy should be an optional dependency.

do we really need this? If someone tries to import the module it already say:

raise ImportError("missing optional dependency 'matplotlib'")

@milancurcic
Copy link
Member

My bad... I got confused about matplotlib-core. Not installed in my venv either.

@philippemiron philippemiron linked an issue Nov 22, 2023 that may be closed by this pull request
clouddrift/__init__.py Outdated Show resolved Hide resolved
@milancurcic
Copy link
Member

@selipot please check on your end if all good, and merge when ready.

clouddrift/plotting.py Show resolved Hide resolved
clouddrift/plotting.py Show resolved Hide resolved
Copy link
Member

@selipot selipot left a comment

Choose a reason for hiding this comment

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

just small editing of the docstring examples needed.

@selipot selipot merged commit 94a5efc into Cloud-Drift:main Nov 27, 2023
15 checks passed
@philippemiron philippemiron deleted the plotting-utils branch November 27, 2023 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants