Richer Unstructured Plotting Support #4109
Conversation
|
The land-mask NaNs are thresholded out, showing a stock image texture map, with labelled parallels and meridians graticule. |
|
The land-mask NaNs are thresholded out, showing a stock image texture map, with labelled parallels and meridians graticule for various "funky" PROJ projections. This animation also highlights cell picking with region statistics i.e., min, max, mean etc |
|
The land-mask NaNs are thresholded out along with Sea Surface Temperature data out side a specific range. Also showing a stock image texture map, with labelled parallels and meridians graticule. As the thresholding applies to different cells for different parts of the time-series, the slider shows the associated interactive mesh animation. |
I'm all for this, especially given Numpy's recent py36 deprecation. But since we know one of Iris' main use cases is currently on py36 environments, are we confident that this will move forward soon? Otherwise I'd consider the lack of py36 support a blocker for merging this feature branch. |
|
Now is the time IMHO. @jamesp What say ye? |
|
See #4108 for avoiding the conda resolve in CI? I'm all for chopping py36 going forwards, but we will need to support it for our legacy environments. I assume those won't go beyond iris 3.0.x though. |
|
The land-mask NaNs are thresholded out along with Sea Surface Temperature data out side a specific range. Also showing labelled parallels and meridians graticule. As the thresholding applies to different cells for different parts of the time-series, the slider shows the associated interactive mesh animation. Underneath a base mesh is rendered. |
|
Looking at the CI tests here though, py37 is timing out |
|
@jamesp Let's get your PR for This seems like the best way forwards here... although, I'm not concerned by the |
@jamesp The release feature branches e.g., I'm more than happy, and relieved, to be dropping |
There was a problem hiding this comment.
OK @bjlittle, here's half my review. I'm yet to review experimental.ugrid.plot; I'll get on that next.
CLI review
Great idea! It's really valuable to enable common operations without going through complexities of Python activation, imports etc. - those are big overheads if all you want is a quick summary or visualisation.
But the proposal here offers way too much configurability - once a user is invested enough to be tweaking configurations, doing so via Python becomes a much smaller relative overhead. So there's less benefit to offering the extra config, but we're still paying some classic future maintenance costs, which aren't worth it:
- Extra code complexity when re-visiting.
- Work to keep up to date with the Python functions/classes being used.
- Minor codebase bloat.
I'd prefer that we keep most of the config in a single place - Python - rather than maintaining a second entry point for so many options. I've marked the options that I therefore think should be removed (and I've skipped testing that those parts of the code work). Sorry for your sunk cost 😔
I can confirm that the remaining config options work as expected 👍
| "http://matthew-brett.github.com/pydagogue/gh_delete_master.html", | ||
| "https://readthedocs.org/dashboard/scitools-iris/version/v3.0.0rc0/", |
There was a problem hiding this comment.
I don't want to merge this PR while it has these explicit ignore commands when we know a fix is incoming in #4104. As a feature branch PR, I'm in favour of merging with link check failures, knowing that the fix is already due in master (and we can do a FB merge back if necessary).
|
|
||
| import click | ||
| from click_default_group import DefaultGroup | ||
| from colorama import Fore, Style |
There was a problem hiding this comment.
Can we drop this? There's a lot of talk about Iris' dependencies being bulky, and a desire to make them leaner. The added eye candy isn't worth it IMO.
| ), | ||
| ) | ||
| @click.argument("filename", type=click.Path(exists=True, dir_okay=False)) | ||
| @main.command("plot") |
There was a problem hiding this comment.
Move this line to the top of the block, as you have done with show and summary - consistency is good and IMO that way round is marginally easier to interpret.
| if part == ":": | ||
| slicer.append(slice(None)) | ||
| elif ":" in part: | ||
| defaults = dict(start=0, stop=extent, step=1) |
There was a problem hiding this comment.
Downstream modification of defaults is counter-intuitive. Could you either use a different name, or copy defaults before modifying?
| # cli: plot | ||
| # | ||
| @click.option( | ||
| "--background-color", |
There was a problem hiding this comment.
Benefit not worth the maintenance.
| ), | ||
| ) | ||
| @click.option( | ||
| "--edge-color", |
There was a problem hiding this comment.
Benefit not worth the maintenance.
| ) | ||
| @click.option( | ||
| "-s", | ||
| "--scalars", |
There was a problem hiding this comment.
Benefit not worth the maintenance.
| @click.option("--show-edges", is_flag=True, help="Render mesh cell edges.") | ||
| @click.option( | ||
| "-v", | ||
| "--view", |
There was a problem hiding this comment.
Benefit not worth the maintenance.
| - nc-time-axis | ||
| - pandas | ||
| - pip | ||
| - pykdtree |
There was a problem hiding this comment.
This should also be included in requirements/all.txt
| "load_cube", | ||
| "load_cubes", | ||
| "load_raw", | ||
| "main", |
There was a problem hiding this comment.
main() was an OK name for the function within iris.experimental.ugrid.cli, but given the wider usage could we come up with something less vague? cli, or cli_main, or something else?
|
There is clearly a lot of useful functionality in this CLI, it provides a good overview and easy access to the new plotting code. But maintaining it won't come for free. Given the limited user feedback we have so far it would make sense to get this in front of people and see if it is useful. If it is, great, we can invest proportionally more of our effort in building out this kind of interface. If it's not used, let's pull it out again and take that as positive feedback to focus our efforts elsewhere. I suggest we make a condition of a future release (tracked either as a github issue/discussion or Jira) that the utility of the CLI has been reviewed. Success criteria could be e.g. we have at least one user who is prepared to take the time to write a testimonial entreating us to retain the CLI. If we can't muster that, then it gets the 🥩. |
|
This PR may have caused some confusion, so apologies for that. Just to be clear - not one line of this PR (mostly It's simply here to bank as part of the internal Aside, from that, this PR has helped me gain a better technical understanding of I've written the code with this all in mind, with the functionality being mainly self-contained within So with regards to |
|
|
||
| def _threshold(mesh, location, threshold, invert): | ||
| """Apply the threshold to the mesh""" | ||
| if isinstance(threshold, bool) and threshold: |
There was a problem hiding this comment.
| if isinstance(threshold, bool) and threshold: | |
| if threshold is True: |
Pretty sure that avoids it working on truth-ish values.
|
|
||
| for lat in lats: | ||
| xyz = to_xyz(np.ones_like(lons) * lat, lons, radius=radius) | ||
| connectivity = np.arange(-1, num) |
There was a problem hiding this comment.
Any chance of not re-using an existing term for this variable name?
| xyz = np.vstack( | ||
| [lons, np.ones_like(lons) * lat, np.zeros_like(lons)] | ||
| ).T | ||
| connectivity = np.arange(-1, num) |
There was a problem hiding this comment.
Any chance of not re-using an existing term for this variable name?
|
|
||
| for lon in lons: | ||
| xyz = to_xyz(lats, np.ones_like(lats) * lon, radius=radius) | ||
| connectivity = np.arange(-1, num) |
There was a problem hiding this comment.
Any chance of not re-using an existing term for this variable name?
| xyz = np.vstack( | ||
| [np.ones_like(lats) * lon, lats, np.zeros_like(lats)] | ||
| ).T | ||
| connectivity = np.arange(-1, num) |
There was a problem hiding this comment.
Any chance of not re-using an existing term for this variable name?
| * step (None or float): | ||
| Specify the increment (in degrees) step size from the equator to the poles, | ||
| used to determine the graticule lines of latitude. The ``step`` is | ||
| modulo ``90`` degrees. Default is ``DEFAULT_LATITUDE_STEP``. |
There was a problem hiding this comment.
This constant isn't part of __all__.
| * lon_step (None or float): | ||
| Specify the increment (in degrees) step size from the prime meridian eastwards, | ||
| used to determine the longitude position of latitude labels. The ``lon_step`` is | ||
| modulo ``180`` degrees. Default is ``DEFAULT_LONGITUDE_STEP``. |
There was a problem hiding this comment.
This constant isn't part of __all__.
| * num (None or float): | ||
| Specify the number of points contained within a graticule line of longitude. | ||
| Default is ``DEFAULT_LONGITUDE_NUM``. |
There was a problem hiding this comment.
This constant isn't part of __all__.
| * step (None or float): | ||
| Specify the increment (in degrees) step size from the prime meridian eastwards, | ||
| used to determine the graticule lines of longitude. The ``step`` is | ||
| modulo ``180`` degrees. Default is ``DEFAULT_LONGITUDE_STEP``. |
There was a problem hiding this comment.
This constant isn't part of __all__.
| "to_xyz", | ||
| ] | ||
|
|
||
| # default graticule latitude parallel linspace num |
There was a problem hiding this comment.
| # default graticule latitude parallel linspace num | |
| # TODO: a thorough review of this module's code. Not enough time at time of writing - 2021-04-28. | |
| # default graticule latitude parallel linspace num |
3 participants
🚀 Pull Request
Description
This PR updates the experimental unstructured plotting support.
Note that, this PR contains highly experimental concept code, in order to demonstrate capability and features from PyVista. It is not tested, but rather "used". Full testing rigor will come later.
This PR touches on quite a few areas, most notably:
iris-pyvista plot ...to render an unstructured cube from fileiris-pyvista show ...to render a VTK fileiris-pyvista summary ...to show a cube summary from fileiris-pyvista --helpfor further detailsiris-pyvista plotis the default sub-commandclimof thecmapto cover the full extent of the data (specifically for a 2D time-series)Consult Iris pull request check list