Skip to content

Clean up NodeDefinition typing, fix empty-input handling, expose ClusterSummaryFeatures#890

Open
sevmag wants to merge 4 commits into
graphnet-team:mainfrom
sevmag:chore/node-definition-cleanup
Open

Clean up NodeDefinition typing, fix empty-input handling, expose ClusterSummaryFeatures#890
sevmag wants to merge 4 commits into
graphnet-team:mainfrom
sevmag:chore/node-definition-cleanup

Conversation

@sevmag
Copy link
Copy Markdown
Collaborator

@sevmag sevmag commented May 18, 2026

Summary

First in a series of small PRs being carved out of #813 to make review easier (see the comment on #813 for the full split plan). This one is all the touch-ups around NodeDefinition that landed in that branch and aren't tied to the CNN work — pulling them out so they can be reviewed and merged on their own.

Three independent commits:

  1. Clean up typing and docstrings in NodeDefinition — replace lowercase torch.tensor annotations with torch.Tensor; fix return-type docstrings on forward and _construct_nodes (they return a tensor, not a graph); correct ClusterSummaryFeatures._construct_nodes return annotation from Data to torch.Tensor; fix _verify_standardization annotation (returns None, not torch.Tensor); reorder imports to project convention; clarify the ClusterSummaryFeatures docstring about which features come from Glauch's thesis and which (counts) don't.
  2. Fix NodeAsDOMTimeSeries empty-input handling — when the input pulse tensor is empty, _construct_nodes previously returned a torch_geometric.data.Data built from np.column_stack([x, []]) instead of a tensor of shape [0, num_features]. Downstream callers expect a tensor; this could crash or silently produce a malformed graph. Returns a correctly-shaped empty tensor and drops the now-unused Data import.
  3. Export ClusterSummaryFeatures from the top-level data_representation package (was already reachable via .graphs).

No functional changes outside of (2), which is a bug fix.

Test plan

  • CI green
  • pre-commit run --files src/graphnet/models/data_representation/graphs/nodes/nodes.py src/graphnet/models/data_representation/__init__.py — already verified locally

Split from #813.

🤖 Generated with Claude Code

@sevmag sevmag mentioned this pull request May 18, 2026
sevmag added 3 commits May 19, 2026 08:57
Replace lowercase `torch.tensor` annotations with `torch.Tensor`, fix
return-type docstrings on `NodeDefinition.forward` and
`_construct_nodes` (they return a tensor, not a graph), correct the
`ClusterSummaryFeatures._construct_nodes` return annotation from
`Data` to `torch.Tensor`, and fix `_verify_standardization`'s
return annotation (it returns None, not a tensor). Reorder imports to
match the project's standard grouping. Also clarify the
`ClusterSummaryFeatures` docstring about which features come from
Glauch's thesis and which (counts) do not.
When the input pulse tensor was empty, `_construct_nodes` returned a
`torch_geometric.data.Data` object built from `np.column_stack([x, []])`
instead of a `torch.Tensor` shaped `[0, num_features]`. Downstream
code expects a tensor, so this could crash or silently produce a
malformed graph. Return a correctly-shaped empty tensor instead, and
drop the now-unused `Data` import.
The class was already importable through
`graphnet.models.data_representation.graphs` but missing from the
top-level `data_representation` package's public re-exports.
@sevmag sevmag force-pushed the chore/node-definition-cleanup branch from 84154ba to 5576fdd Compare May 19, 2026 12:57
Copy link
Copy Markdown
Collaborator

@Aske-Rosted Aske-Rosted 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 me as long as there is a reason for why you might want to allow the passing of empty data objects. I would assume that we in that case might just want to catch that instance and raise an error?

@sevmag
Copy link
Copy Markdown
Collaborator Author

sevmag commented May 26, 2026

This looks good to me as long as there is a reason for why you might want to allow the passing of empty data objects. I would assume that we in that case might just want to catch that instance and raise an error?

Good point! I agree that an empty graph is probably not what we want. I will adjust it to throw an error.

@sevmag sevmag requested a review from Aske-Rosted May 26, 2026 23:56
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.

2 participants