[feature] add trace_over to superimpose a particle trajectory to a heatmap

[DavidFang03]

z_part = PartQuantity(
    "PART_X3",
    r"$z^\mathrm{part}$",
)

custom_partQuantities = [z_part]

analytical_trajectory.plot_kwargs = {"ls": "--"}

SpaceTimeHeatmaps = [
    SpaceTimeHeatmap(
        "Dust0_RHO",
        r"$\rho^{dust}$",
        plot_coords=[0, 0],
        trace_over=[z_part],
        ref_function=analytical_trajectory,
    )
]

[Copilot]

Pull request overview

Adds support for overlaying one or more particle time-series trajectories on top of a SpaceTimeHeatmap, so particle paths can be superimposed on x–t heatmaps (optionally alongside analytical reference curves).

Changes:

• Add trace_over configuration to SpaceTimeHeatmap and render those traces as line overlays on the heatmap.
• Auto-add trace_over particle quantities into Pipeline.partQuantities and skip rendering them in the standalone particle time-series plot.
• Make Data.plot_coords optional by providing a default.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
idefix2python/renderer.py	Draws trace_over trajectories on space-time heatmaps and skips rendering “trace-only” particle quantities in the time-series figure.
idefix2python/core.py	Adds trace_over to SpaceTimeHeatmap, changes Data.plot_coords defaulting, and auto-injects trace quantities into partQuantities.

Comments suppressed due to low confidence (1)

idefix2python/core.py:98

• plot_coords now defaults to a mutable list. Because default argument values are shared between calls, mutating plot_coords on one instance can affect others. Use an immutable default (e.g., tuple) or default to None and assign [0, 0] inside the constructor.

    def __init__(self, key, symbol, plot_coords=[0, 0], vmin=None, vmax=None, **kwargs):
        self.key = key
        self.symbol = symbol
        self.plot_coords = plot_coords
        self.bounds = [vmin, vmax]

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This loop appends into the caller-provided partQuantities list. With the current partQuantities=[] default in the signature, this can leak state across Pipeline instances and also unexpectedly mutate caller-owned lists. Also, it iterates over the raw spaceTimeHeatmaps argument (which may be a dict after _to_dict conventions), so passing a dict would iterate keys and break on heatmap.trace_over; iterate over self.spaceTimeHeatmaps.values() (or normalize to a list) and copy partQuantities before mutating.

Suggested change

	for heatmap in spaceTimeHeatmaps:
	for traceover in heatmap.trace_over:
	if isinstance(traceover, PartQuantity):
	if traceover.key not in self.spaceTimeHeatmaps:
	traceover.is_trace_over = True
	partQuantities.append(traceover)
	self.partQuantities = _to_dict(partQuantities)
	normalized_part_quantities = list(partQuantities)
	for heatmap in self.spaceTimeHeatmaps.values():
	for traceover in heatmap.trace_over:
	if isinstance(traceover, PartQuantity):
	if traceover.key not in self.spaceTimeHeatmaps:
	traceover.is_trace_over = True
	normalized_part_quantities.append(traceover)
	self.partQuantities = _to_dict(normalized_part_quantities)

[Copilot]

The is_trace_over flag is set based on traceover.key not in self.spaceTimeHeatmaps, which doesn’t reflect whether the user explicitly requested this PartQuantity as a time series. As written, a user-supplied PartQuantity that is also used in trace_over will be hidden from render_timeSeries(). Track whether the quantity was auto-added vs user-provided (e.g., check membership in the original partQuantities before appending) and only set is_trace_over for auto-added items.

Suggested change

	for heatmap in spaceTimeHeatmaps:
	for traceover in heatmap.trace_over:
	if isinstance(traceover, PartQuantity):
	if traceover.key not in self.spaceTimeHeatmaps:
	traceover.is_trace_over = True
	partQuantities.append(traceover)
	original_part_quantity_keys = set(_to_dict(partQuantities).keys())
	for heatmap in spaceTimeHeatmaps:
	for traceover in heatmap.trace_over:
	if isinstance(traceover, PartQuantity):
	if traceover.key not in original_part_quantity_keys:
	traceover.is_trace_over = True
	partQuantities.append(traceover)

[Copilot]

SpaceTimeHeatmap now always has a trace_over attribute, so hasattr(field1D, "trace_over") is redundant. Also, calling ax.legend() inside the loop rebuilds the legend for every trace; add all lines first and call legend() once (or only if any labeled artists were added) to reduce overhead and keep ordering stable.

Suggested change

	if len(field1D.pointsRef) > 0:
	ax.plot(field1D.pointsRef, field1D.valuesRef, label="Predicted")
	ax.legend()
	if hasattr(field1D, "trace_over"):
	for trace_over in field1D.trace_over:
	ax.plot(trace_over.points, trace_over.values, label=trace_over.key)
	ax.legend()
	has_legend_items = False
	if len(field1D.pointsRef) > 0:
	ax.plot(field1D.pointsRef, field1D.valuesRef, label="Predicted")
	has_legend_items = True
	for trace_over in field1D.trace_over:
	ax.plot(trace_over.points, trace_over.values, label=trace_over.key)
	has_legend_items = True
	if has_legend_items:
	ax.legend()

[Copilot]

Using hasattr(qty, "is_trace_over") will skip any quantity that happens to have that attribute, even if it’s False in the future. Prefer an explicit boolean check like if getattr(qty, "is_trace_over", False): (and ideally define is_trace_over: bool = False on PartQuantity rather than monkey-patching it in Pipeline).

Suggested change

	if hasattr(qty, "is_trace_over"):
	if getattr(qty, "is_trace_over", False):

[DavidFang03]

@copilot

[Copilot]

@copilot

Addressed all four review points in commit 836985a:

• Mutable default arguments: Changed spaceTimeHeatmaps=[], movies1D=[], movies2D=[], partQuantities=[] to None in Pipeline.__init__, using or [] internally.
• Caller list mutation: partQuantities is now copied with list(partQuantities or []) before appending trace-over items, so caller-owned lists are never mutated.
• is_trace_over on PartQuantity: Added self.is_trace_over = False to PartQuantity.__init__ so it's a proper instance attribute rather than monkey-patched later.
• hasattr vs getattr: The renderer already uses getattr(qty, "is_trace_over", False) (was addressed in a prior commit).