Release/2.5 - #39
Conversation
fix/using np.nan as np.NaN was removed in the NumPy 2.0 release
…s_with_mastu_data Improve failsafe for plotequicomp when data is missing
…nd_magnetic_axes_in_plotequilibrium
…nd_magnetic_axes_in_plotequilibrium
Bumps the actions-deps group with 4 updates: [actions/checkout](https://github.com/actions/checkout), [actions/setup-python](https://github.com/actions/setup-python), [actions/upload-artifact](https://github.com/actions/upload-artifact) and [actions/download-artifact](https://github.com/actions/download-artifact). Updates `actions/checkout` from 4 to 6 - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v4...v6) Updates `actions/setup-python` from 4 to 6 - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@v4...v6) Updates `actions/upload-artifact` from 4 to 7 - [Release notes](https://github.com/actions/upload-artifact/releases) - [Commits](actions/upload-artifact@v4...v7) Updates `actions/download-artifact` from 4 to 8 - [Release notes](https://github.com/actions/download-artifact/releases) - [Commits](actions/download-artifact@v4...v8) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions-deps - dependency-name: actions/setup-python dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions-deps - dependency-name: actions/upload-artifact dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions-deps - dependency-name: actions/download-artifact dependency-version: '8' dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions-deps ... Signed-off-by: dependabot[bot] <support@github.com>
* use polycollection from matplotlib and use cells instead of nodes * Use Outer midplane, Outer divertor target, Inner divertor target values and plot the same
…ith reddish color
…added --no-overlay option. default it will show overlays. added provennace as title
…in_plotequilibrium
…try for machine description
Bumps the actions-deps group with 1 update: [actions/checkout](https://github.com/actions/checkout). Updates `actions/checkout` from 6 to 7 - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v6...v7) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions-deps ... Signed-off-by: dependabot[bot] <support@github.com>
…ovenance-option-to-all-plot-script Generalize no-provenance option to all plotting script
Bumps the actions-deps group with 1 update: [actions/setup-python](https://github.com/actions/setup-python). Updates `actions/setup-python` from 6 to 7 - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@v6...v7) --- updated-dependencies: - dependency-name: actions/setup-python dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major dependency-group: actions-deps ... Signed-off-by: dependabot[bot] <support@github.com>
* Let matplotlib chooses its default backend * Update idstools/view/common.py Co-authored-by: Simon Pinches <SimonPinches@users.noreply.github.com> * added documentation and fix interactive logic * do not save if interactive backend is available * moved backend setup and Jupyter detection into shared module and delayed Matplotlib imports so --rc backend=... is applied before Matplotlib loads. * fix(matplotlib): add backend config helper --------- Co-authored-by: Simon Pinches <SimonPinches@users.noreply.github.com>
…uda uri (#38) * show warning/exception from IMAS-Python when time is not readble when using UDA uri * add cache_mode=none when not used in UDA URI * Moved URI handling into utils, simplified it using urllib.parse, added a reusable backend-specific query helper, automatically appended cache_mode=none to UDA URIs * removed add_default_uda_cache_mode function, simplified _replace and updated documentation
|
@SimonPinches in the EB script associated with this release, we need in the module to either |
There was a problem hiding this comment.
I found three user-facing crashes in code paths this release ships (empty scalar leaf in idsprint, GGD data without a "cells" subset in plotedgeprofiles, and dbperf on entries with no readable IDS), plus a few behavioural changes worth either fixing or calling out in the release notes (forced cache_mode=none on UDA and the silent removal of --show-labels). Details in the line comments.
| elif isinstance(structure, imas.ids_primitive.IDSPrimitive): | ||
| tree = Tree(f"[magenta]{type(structure)}") | ||
| if not structure.has_value: | ||
| tree.add(f"[grey62]{structure.metadata.name}[/]") | ||
| else: | ||
| value = Pretty(structure.value) | ||
| txt = f"[bright_yellow]{structure.metadata.name}[/]:" | ||
| group = Columns([txt, value]) | ||
| if compact: | ||
| group = Columns([f"[bright_yellow]{structure.metadata.name}[/]"]) | ||
| tree.add(group) | ||
| return tree |
There was a problem hiding this comment.
Bug (crash + dropped output). value is only assigned in the else branch, so for an empty primitive (has_value == False) the Columns([txt, value]) line raises UnboundLocalError. And when the primitive does have a value, tree.add(group) only happens inside if compact: — in the default (non-compact) mode the value is built and then silently discarded, so idsprint shows only the type header.
| elif isinstance(structure, imas.ids_primitive.IDSPrimitive): | |
| tree = Tree(f"[magenta]{type(structure)}") | |
| if not structure.has_value: | |
| tree.add(f"[grey62]{structure.metadata.name}[/]") | |
| else: | |
| value = Pretty(structure.value) | |
| txt = f"[bright_yellow]{structure.metadata.name}[/]:" | |
| group = Columns([txt, value]) | |
| if compact: | |
| group = Columns([f"[bright_yellow]{structure.metadata.name}[/]"]) | |
| tree.add(group) | |
| return tree | |
| elif isinstance(structure, imas.ids_primitive.IDSPrimitive): | |
| tree = Tree(f"[magenta]{type(structure)}") | |
| if not structure.has_value: | |
| tree.add(f"[grey62]{structure.metadata.name}[/]") | |
| return tree | |
| if compact: | |
| group = Columns([f"[bright_yellow]{structure.metadata.name}[/]"]) | |
| else: | |
| txt = f"[bright_yellow]{structure.metadata.name}[/]:" | |
| group = Columns([txt, Pretty(structure.value)]) | |
| tree.add(group) | |
| return tree |
| polygons, cell_values = self.edge_profiles_compute.get_cell_polygons_and_values(time_slice, ne_values) | ||
| all_r = np.concatenate([v[:, 0] for v in polygons]) |
There was a problem hiding this comment.
Bug (crash on missing "cells" grid subset). get_cell_polygons_and_values() is documented to return (None, None) when the "cells" grid subset is unavailable, but this immediately iterates polygons → TypeError: 'NoneType' object is not iterable, so the graceful "no data" branch below never runs. Same pattern in view_ion_density (line 177) and view_neutral_density (line 226).
| polygons, cell_values = self.edge_profiles_compute.get_cell_polygons_and_values(time_slice, ne_values) | |
| all_r = np.concatenate([v[:, 0] for v in polygons]) | |
| polygons, cell_values = self.edge_profiles_compute.get_cell_polygons_and_values(time_slice, ne_values) | |
| if polygons is None or len(polygons) == 0: | |
| logger.warning("edge_profiles: no cell polygons available, cannot plot electron density") | |
| return None | |
| all_r = np.concatenate([v[:, 0] for v in polygons]) |
(Ideally the empty-data text shown in the existing else branch would be reused, but the guard above at least prevents the crash. Applies to all three density views.)
| _dummy_hdfde = imas.DBEntry( | ||
| imas.ids_defs.HDF5_BACKEND, args.database, pulse, run, args.user, data_version=args.version | ||
| ) | ||
| _dummy_hdfde.open() | ||
| availables_ids = get_available_ids_and_occurrences(_dummy_hdfde) | ||
| for idsname, occurrence in availables_ids: | ||
| try: | ||
| _dummy = _dummy_hdfde.get(idsname, lazy=True, autoconvert=False) | ||
| src_dd_version = _dummy.ids_properties.version_put.data_dictionary.value | ||
| break | ||
| except Exception as e: | ||
| pass | ||
| _dummy_hdfde.close() |
There was a problem hiding this comment.
Bug (NameError on sparse entries). If the entry contains no readable IDS (empty availables_ids, or every get() raises), src_dd_version is never assigned and the DBEntry(..., dd_version=src_dd_version) calls below raise NameError. Initialising it to None falls back to the default DD, which restores the previous behaviour. The unused except Exception as e will also trip flake8 (F841).
| _dummy_hdfde = imas.DBEntry( | |
| imas.ids_defs.HDF5_BACKEND, args.database, pulse, run, args.user, data_version=args.version | |
| ) | |
| _dummy_hdfde.open() | |
| availables_ids = get_available_ids_and_occurrences(_dummy_hdfde) | |
| for idsname, occurrence in availables_ids: | |
| try: | |
| _dummy = _dummy_hdfde.get(idsname, lazy=True, autoconvert=False) | |
| src_dd_version = _dummy.ids_properties.version_put.data_dictionary.value | |
| break | |
| except Exception as e: | |
| pass | |
| _dummy_hdfde.close() | |
| src_dd_version = None | |
| _dummy_hdfde = imas.DBEntry( | |
| imas.ids_defs.HDF5_BACKEND, args.database, pulse, run, args.user, data_version=args.version | |
| ) | |
| _dummy_hdfde.open() | |
| availables_ids = get_available_ids_and_occurrences(_dummy_hdfde) | |
| for idsname, occurrence in availables_ids: | |
| try: | |
| _dummy = _dummy_hdfde.get(idsname, lazy=True, autoconvert=False) | |
| src_dd_version = _dummy.ids_properties.version_put.data_dictionary.value | |
| break | |
| except Exception: | |
| continue | |
| _dummy_hdfde.close() |
| if profiles2d.grid_type.index == 1 and np.size(r2d) == 0: | ||
| logger.warning( | ||
| f"profiles_2d[{profiles2d_index}].r is not available and grid type is 1.. Calculating from grid" | ||
| if profiles2d.grid_type.index == 1 and profiles2d.grid.dim1 is not None and profiles2d.grid.dim2 is not None: |
There was a problem hiding this comment.
Ineffective check + regression risk. Two points here:
profiles2d.grid.dim1 is not Noneis never False for IMAS array nodes — an equilibrium withgrid_type == 1but emptygrid.dim1/dim2sails through and produces an empty contour. A length check does what was intended:
| if profiles2d.grid_type.index == 1 and profiles2d.grid.dim1 is not None and profiles2d.grid.dim2 is not None: | |
| if profiles2d.grid_type.index == 1 and len(profiles2d.grid.dim1) > 0 and len(profiles2d.grid.dim2) > 0: |
- Previously this function also supported entries that filled 2-D
profiles_2d.r/zdirectly (it fell back to them, or built the mesh from dim1/dim2 only whenrwas empty). Those entries now get "Only rectangular cylindrical grid (grid_type=1) is supported" andNone. If that's intentional, worth a line in the release notes. Also: the returned keys are still namedr2d/z2dbut now hold 1-D arrays — fine for the in-repocontour(r, z, psi.T)call sites (all updated, I checked), but misleading for any external consumer; consider renaming tor/zor noting it in the docstring.
| bnd_type = int(bnd.type) | ||
| if _valid_scalar(bnd_type): |
There was a problem hiding this comment.
Nit: integer fill value passes validation. _valid_scalar uses abs(EMPTY_FLOAT) ≈ 9e40 as threshold, so EMPTY_INT (−999999999) passes and bnd_type can be set to the fill value; the separatrix fallback below then silently takes the limiter/lcfs path. Suggest an explicit check:
| bnd_type = int(bnd.type) | |
| if _valid_scalar(bnd_type): | |
| bnd_type = int(bnd.type) | |
| if bnd_type != imas.ids_defs.EMPTY_INT and _valid_scalar(bnd_type): |
| from idstools.utils.idslogger import setup_logger | ||
| from idstools.view.common import PlotCanvas | ||
| from idstools.utils.utility_functions import add_query_to_uri | ||
| from idstools.view.common import PROVENANCE_TITLE_STYLE, PlotCanvas |
There was a problem hiding this comment.
Nit: duplicate import (F811) — duplicate of the (deliberately early) import on line 15.
| from idstools.view.common import PROVENANCE_TITLE_STYLE, PlotCanvas |
| parser.add_argument( | ||
| "--show-labels", | ||
| help="Show labels", | ||
| "--debug", |
There was a problem hiding this comment.
Silent CLI removal. --rho→--phi and --plots→--profiles are covered in the docs, but --show-labels was removed without a mention — existing scripts using it will now exit with an argparse error. Worth a line in the 2.5.0 release notes (or keeping it as a deprecated no-op for one release).
| legend.legend_handles[leg_index].set_alpha(1.0 if visible else 0.3) | ||
| ax.figure.canvas.draw_idle() | ||
|
|
||
| ax.figure.canvas.mpl_connect("pick_event", on_legend_click) |
There was a problem hiding this comment.
Handler accumulation + function size. Each call to view_magnetic_poloidal_flux registers a new pick_event handler on the figure — and plotequilibrium calls it twice (base plot + create_overlays), so handlers accumulate. Today the stale handler's leg_map misses the new legend artists so it's benign, but it's a leak pattern that will bite as soon as artists are reused. Suggest connecting once per figure (e.g. keep the cid on the figure and disconnect before reconnecting).
More broadly, this function has grown to ~380 lines doing contours, six kinds of overlay, annotations and an interactive legend — extracting the overlay/legend sections into helpers would make it much easier to maintain.
| electron_density[self.nrho + i] = _density[i] | ||
| else: | ||
| _gset = self.edge_profiles.ggd[self.time_index_edge_profiles].electrons.density[self.gset].values | ||
| if len() < 1: |
There was a problem hiding this comment.
Pre-existing bug worth a follow-up issue. Not introduced by this PR, but visible in the surrounding context of the np.nan changes: there are several bare len() calls (if len() < 1: — lines 839, 878, 908, 915, 1092) in the edge-profiles branches. These raise TypeError whenever those paths execute, so the fallbacks they guard can never work. Suggest a follow-up issue rather than folding into this release PR.
| if len(ne_values) != len(r): | ||
| logger.warning( | ||
| f"edge_profiles: per-subset density count {len(ne_values)} != element count {len(r)} " | ||
| f"for subset {subset_identifier_index}, falling back to node interpolation" |
There was a problem hiding this comment.
Nit: misleading log message. The message says "falling back to node interpolation", but ne_list is only populated when no per-subset density entry was found — when we reach this branch (entry found but wrong length) it is always empty, so the function returns (None, None, None) despite the message. Either build the node fallback in this case too, or reword the message.
Notes for release 2.5.0
New features
Improvements
Bug fixes
np.NaNwas removed in the NumPy 2.0 release. Usenp.naninstead. #10 )