Repository navigation
Conversation
Adds Neptune (arXiv:2510.01733) as a backbone in graphnet.models.transformer, together with the components it needs: - FPSTokenizer: farthest-point-sampling tokenizer over the 4D (x, y, z, t) point cloud, with Voronoi or kNN pooling and no cap on pulses per event. - Farthest point sampling with fused assign/kNN variants: a pure-PyTorch backend and an optional Triton backend used automatically on CUDA. - FourierPositionEncoder, RMSNorm, SwiGLU, RoPE4D, AttentionPool, and the Neptune transformer encoder layers. - pack/unpack/build_block_mask helpers for variable-length packing with flex_attention, used at low batch occupancy; padded SDPA otherwise. - DropPath gains optional doc_id/num_docs arguments so stochastic depth stays per-event on the packed path. Default behaviour is unchanged. - has_triton_package() import helper. Also adds a training example (09_train_neptune_model.py) and a test module covering the model, tokenizer, and FPS kernels against brute force. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`FourierEncoder` assumed its input was ordered (x, y, z, time, charge, auxiliary) and applied multipliers fixed to the Kaggle dataset's normalisation, so it index-errors or silently mis-encodes on data laid out differently (issue graphnet-team#753). The multipliers are the more subtle half: they set which separations the sinusoidal ladder can resolve, so carrying them to a detector with a different normalisation changes the resolved physical range without any error. Rename the existing class to `FourierEncoderEPJC`, documented as the EPJ-C Kaggle implementation with those assumptions, and add a `FourierEncoder` taking `schema: Dict[int, float]` -- column index to multiplier. Nothing is assumed about column order or meaning, and the boolean embedding and MLP projection are left to the model, both being problem-specific. Follows the design proposed in the review of graphnet-team#759. The refactor is representation-preserving: the sinusoidal encoding is elementwise, so one embedding per column lays channels out exactly as the strided xyz block did. `tests/models/test_embedding.py` asserts that, and that a schema of the EPJC columns reproduces its pre-projection embedding bit-for-bit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F75gXoxTcs4nVRxGp91ENf
`seq_length` is a per-sequence quantity, while `x` is per-step, and the name is also the constructor's embedding-width argument -- so state the `[B]` shape and that each step receives a copy of its sequence's embedded length. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F75gXoxTcs4nVRxGp91ENf
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F75gXoxTcs4nVRxGp91ENf
A column's resolvable band has two ends: the multiplier fixes the finest wavelength, `n_freq` fixes how far the ladder reaches beyond it. Only the first was per column, so features wanting different spans could not get them -- and they do differ. On a 1 km neutrino detector the sensor spacing and extent ask for a span of ~180x, event timing for ~3600x and the charge quantisation for ~25x, against the single hardcoded 10000 (a ~9085x ladder) every feature shares today. `schema` now takes either a multiplier or a `(multiplier, n_freq)` pair per column, with a bare multiplier falling back to the encoder's `n_freq`, so existing schemas are unchanged. One `SinusoidalPosEmb` is built per distinct span rather than per column, which keeps the common single-span case at one module and one learnable scale under `scaled`. `SinusoidalPosEmb` also precomputes its ladder into a non-persistent buffer instead of rebuilding it every forward pass, so the extra spans cost nothing and checkpoints are untouched. Its `n_freq` docstring said "Number of frequencies", which is `dim / 2`; it is the span of the ladder, and that mislabel is what hid the missing knob. Verified bit-identical against the previous implementation for the ladder itself, `FourierEncoderEPJC`, `SpacetimeEncoder`, and `FourierEncoder` with bare multipliers; a per-column span changes only its own block. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F75gXoxTcs4nVRxGp91ENf
Resolve the components/layers.py conflict by adopting upstream's removal of the module and moving the Neptune layer classes (RMSNorm, SwiGLU, RoPE4D, AttentionPool, NeptuneTransformerEncoderLayer, NeptuneTransformerEncoder) to a new components/neptune_layers.py, matching the per-family module layout introduced upstream. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| for cross-attention pooling with a learned query. | ||
| layerscale_init: Initial value of the LayerScale parameters. | ||
| attn_impl: `"auto"`, `"padded"`, or `"packed"`. Purely a | ||
| throughput choice; all three are mathematically equivalent. |
There was a problem hiding this comment.
Make sure to explain what each of the settings means. It should be as concise as possible but still descriptive enough so that people actually understand what you meant by it. The way it is done now is that it is not explained here, but then later on there is a very long comment explaining it, which, imo, is suboptimal
There was a problem hiding this comment.
The modes are now documented in the argument's docstring; the long comment is gone.
sevmag
left a comment
There was a problem hiding this comment.
@felixyu7 we are honored that this model gets to live in graphnet. The implementation is mostly carefully crafted by an eager helper (@claude). General stlye comments:
- the code is veeeeery verbose, reviewing so many lines of code in one PR is very exhausting. For these kinds of repos like graphnet manual code review is still necessary to some extend. Not for correctness but mainly to make sure it is maintainable
- comments in the code should ideally not span multiple lines (more than 2-3)
- docstrings should be held to the minimum that is need to understand the code from the context but should also explain the most important parts
More concrete desgin comments:
- the data representation stuff like standardization or node sampling should probably be done in the code that lies in graphnet/models/data_representation and not in the model forward
- I do not think we should add the triton kernel to. the main branch for now. See my comment about this at the code for alternatives
- we should aim for a general version of the FourierEncoder (see #913 ) before we start introducing a new class
Apart from that the PR looks very good! Make sure to adjust the comments or state your point against them and try to deverbose as much as possible!
|
|
||
| self.token_dim = token_dim | ||
| self.pool_type = pool_type | ||
| # "auto": packed flex on CUDA, padded SDPA on CPU (where the packed |
There was a problem hiding this comment.
The modes are now documented in the argument's docstring; the long comment is gone.
|
|
||
| def __init__( | ||
| self, | ||
| token_dim: int = 768, |
There was a problem hiding this comment.
why do we not call this d_model as it is called in NeptuneTransformerEncoderLayer, that way it is less confusing
| `[B, token_dim]` pooled event representation. | ||
| """ | ||
| # No cast to `tokens.dtype`: the position encoder computes in float32 | ||
| # anyway, and a bfloat16 round-trip of km-scale coordinates costs up |
There was a problem hiding this comment.
this comment is a little bit overly specific. I think it confuses more than it helps. While I do agree that we should mention that it is on purpose the we do not cast to a specific numerical type, I think we should reframe this.
E.g. : "# Positions are keept in float32 in purpose to avoid loss of information due to rounding"
or something shorter and less speficic (The rad number is maybe a bit too much)
| embeddings. This class embeds *position only*, and is applied to token | ||
| centroids rather than to individual pulses. | ||
|
|
||
| Time is deliberately excluded: callers are expected to centre time per |
There was a problem hiding this comment.
I get what this argument is saying, but feeding the time feature which is centered on the charge-weighted mean time is not meaningless at all it gives information on whether a pulse is early or late in the event. I do not think this argument actually holds.
There was a problem hiding this comment.
Agreed, that argument was wrong. It's now framed as a design choice: time is encoded relatively via RoPE, and adding column 3 to position_encoding_schema also encodes it absolutely.
| """ | ||
| # Trig in float32 for accuracy; the MLP follows the ambient autocast | ||
| # dtype. | ||
| xyz = coords[..., : self.in_dim].float() |
There was a problem hiding this comment.
this assumes that the coordinates are the first self.in_dim feature columns, which is not ideal. Let's pass a list of indices that are the indices of the position features in the feature matrix, so that this is more general.
There was a problem hiding this comment.
The class is removed in favour of #913's FourierEncoder, whose schema names the columns explicitly and builds the frequencies once.
| @@ -0,0 +1,826 @@ | |||
| """Learnable tokenizers turning point clouds into transformer tokens.""" | |||
There was a problem hiding this comment.
This whole file is really long. Let's de-verbose it; otherwise, reviewing 900 lines in one file is a bit tideous
There was a problem hiding this comment.
826 → 519 lines; the duplicated summary-statistics code is now shared.
| from graphnet.models.utils import flex_attention | ||
|
|
||
|
|
||
| class RMSNorm(LightningModule): |
There was a problem hiding this comment.
can we not use the torch native RMSNorm https://docs.pytorch.org/docs/2.14/generated/torch.nn.RMSNorm.html ?
There was a problem hiding this comment.
I don't think we should introduce triton kernels in graphnet itself as I'm not sure whether people will maintain it and it takes a lot of expertise to do so. I think we should stick with the slower, normal Python code, and if people want, they can implement the Triton kernel in their own branch (you could link a piece of code that does this). The other option is that you write an outside module that we can keep as an optional dependency as we are doing with jammy_flows. then peopel can use it if they want without us having the implementation in the graphnet code
| return False | ||
|
|
||
|
|
||
| def has_triton_package() -> bool: |
There was a problem hiding this comment.
see comment for triton kernel of fps
| return rw_landing | ||
|
|
||
|
|
||
| # ------------------------------------------------------------------------ |
…-backbone Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the thorough review! I agree with all of it, and the branch now addresses every comment:
@sevmag ready for another look whenever you have time. |
|
I also added the numerics guard in the Sinusodial embedding that you were asking about! LMK if you have other things you want to add |
# Conflicts: # src/graphnet/models/components/embedding.py # src/graphnet/models/gnn/icemix.py
|
@sevmag should be done, let me know if there are any issues! |
There was a problem hiding this comment.
Some of these components are general concepts like SwiGLU, 4DRoPE, and attention pool. Lets move them in more general files (not having Neptune in the title) as other people might want to reuse these for their models and then neptune_layers.py is a poor name for the file.
I'm actually also not sure whether we need so many very specific classes like NeptuneTransformerEncoder and PointTransformerEncoder in neptune.py. I think we can make this easier and have the Neptune class in neptune.py do a bit more parsing and work but there being able to remove these medium-level classes. This should also save complexity. I'm also not the biggest fan of the usage of the factory functions here since they complicate the interfaces. Lets stick to args
There was a problem hiding this comment.
Done! SwiGLU, AttentionPool and the encoder block (now RotaryTransformerBlock) are in attention_blocks.py, RoPE4D is in embedding.py, and neptune_layers.py is gone. Both encoder wrappers and the layer factory are removed too, so Neptune builds its layers directly from args.
There was a problem hiding this comment.
see my comment in neptune_layers. The usage of PointTransformerEncoder might be unnecessary
| if compile_encoder: | ||
| self.compile_layers() | ||
|
|
||
| def compile_layers( |
There was a problem hiding this comment.
I think we can get rid of this function and just put the code in the init call saves a lot of lines and makes everything more readable
| """ | ||
| x, batch, batch_size = data.x, data.batch, data.num_graphs | ||
| if self._charge_index is None: | ||
| charge = x.new_ones(x.shape[0]) |
There was a problem hiding this comment.
what is the case for? Is this maybe a scenario where there are no charge variables so you asign a charge of 1? if yes this should not be done in the model but in the data_representation
There was a problem hiding this comment.
Yep, that's the no-charge case. I'd keep it in the model though: the tokenizer uses charge for weighting and token summaries, and for Prometheus every pulse is a single photon hit, so unit charge is the physically correct value rather than a placeholder. Moving it to the data representation would mean a new NodeDefinition just to append a column of ones. Happy to make charge_column required instead if you'd prefer.
|
|
||
| # Packing of variable-length batches for block-diagonal `flex_attention`. | ||
| # Compiled on first use, ~50x faster than eager `create_block_mask`. | ||
| _COMPILED_CREATE_BLOCK_MASK: Optional[Callable] = None |
There was a problem hiding this comment.
wait do you use this global to set compiling??
There was a problem hiding this comment.
Nope, it only cached the compiled create_block_mask so it compiles once. It's a plain module-level torch.compile(...) now (lazy, compiles on first call), so the global is gone.
| doc_id: `[N]` event index of every packed token. | ||
| n_tokens: Packed sequence length `N`. | ||
| """ | ||
| global _COMPILED_CREATE_BLOCK_MASK |
| ) -> Tensor: | ||
| return doc_id[q_idx] == doc_id[kv_idx] | ||
|
|
||
| return _COMPILED_CREATE_BLOCK_MASK( |
There was a problem hiding this comment.
when would you not use torch.compile?
There was a problem hiding this comment.
Fair point. Compile is now on by default (compile_encoder=True), applied per layer, which also cut warmup from ~42s to ~9s at the same step time. Turn it off only for CPU-only or debugging runs (the tests do, to keep CI fast). I also dropped attn_impl: packed flex attention now runs automatically when compiled on GPU.
|
|
||
| Ported from the Neptune reference implementation | ||
| (https://github.com/felixyu7/neptune, MIT licence). Calls go to the optional | ||
| `torch_fps` package (Triton kernels on CUDA, C++ on CPU) when it is installed; |
There was a problem hiding this comment.
make sure to link this repo so people can find it
There was a problem hiding this comment.
also create a release so that it is backwards compatible no matter what you do with it after
There was a problem hiding this comment.
Tagged and released v0.6.0 (same code as PyPI 0.6.0): https://github.com/felixyu7/torch-fps/releases/tag/v0.6.0. The install hint now pins torch-fps==0.6.*.
| start_idx: Optional[Tensor], | ||
| random_start: bool, | ||
| generator: Optional[torch.Generator], | ||
| precision: Optional[torch.dtype], |
There was a problem hiding this comment.
Is there a good reason for why precision should be optional? Silent skips of dtypes are annoying to debug
There was a problem hiding this comment.
No good reason. Neptune never set it, and the fallback silently upcast half precision while torch_fps didn't. Removed the arg, so both backends always compute in float32.
Move SwiGLU, AttentionPool and the rotary block (now RotaryTransformerBlock) into attention_blocks.py and RoPE4D into embedding.py; delete neptune_layers.py. Drop the PointTransformerEncoder/NeptuneTransformerEncoder wrappers and the layer factory so Neptune builds its layers from args. Compile per layer by default (compile_encoder=True), dropping attn_impl and compile_layers. Replace the global block-mask cache with a module-level torch.compile. Remove the FPS precision argument; both backends compute in float32. Link and pin torch-fps 0.6. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Compiling each layer separately keyed one dynamo cache entry per layer on their shared forward (each layer has its own drop-path rate). With the eval-mode entries that overran the recompile limit, so the deeper layers silently ran eager, where packed flex attention materialises the full N x N mask: training ran 2.6x slower on an H200 (12.3 vs 31.5 batch/s). Compile one module-level function over the whole stack instead, and test that a stack deeper than the limit compiles in train and eval. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
Adds Neptune ("a N Efficient Point Transformer for Ultrarelativistic Neutrino Events", arXiv:2510.01733, reference implementation) as a backbone in
graphnet.models.transformer.Neptune treats an event as an irregular 4D
(x, y, z, t)point cloud. It tokenizes the event by farthest point sampling (FPS) into at mostnum_patchescentroids, pools the pulses of each token, runs a rotary-attention transformer over the tokens, and pools to one event vector. It has no edges and no pulse cap, so it drops into aStandardModelwithEdgelessGraphoverNodesAsPulses.Builds on #913 (generalised
FourierEncoder); this PR will be rebased once #913 merges.What is added
models/transformer/neptune.py:Neptune(GNN)andPointTransformerEncoder. Columns are passed by name (coordinate_columns,time_column,charge_column,feature_columns) and resolved againstinput_feature_names. The model takes theDetector's standardized inputs as-is; geometric defaults are inIceCube86units, and the charge column must be physical (Detector(replace_with_identity=["charge"])). Centroid positions are encoded with Generalise FourierEncoder with a column/frequency schema #913'sFourierEncoder.components/tokenizers.py:FPSTokenizer. Small events keep one token per pulse; larger ones are reduced to centroids and pooled by Voronoi assignment or kNN gather.components/fps.py:farthest_point_sampling,farthest_point_sampling_with_assign,farthest_point_sampling_with_knn,nearest_assignin pure PyTorch. If the optionaltorch-fpspackage is installed, these delegate to it (Triton on CUDA, C++ on CPU).components/neptune_layers.py:SwiGLU,RoPE4D,AttentionPool,NeptuneTransformerEncoderLayer,NeptuneTransformerEncoder.models/utils.py:pack/unpack/build_block_maskfor variable-length packing withflex_attentionat low batch occupancy.utilities/imports.py:has_torch_fps_package().DropPath.forwardgains optionaldoc_id/num_docsso stochastic depth stays per-event on the packed path; default behaviour is unchanged.examples/04_training/09_train_neptune_model.py: direction reconstruction on the Prometheus example data.tests/models/test_neptune.py: tests for shapes, empty events and batches, gradients, feature-name resolution, time-shift invariance, low precision, config round trip,StandardModel, and FPS/kNN/assign against brute force and againsttorch-fps.Dependencies
No new required dependencies.
torch-fpsis optional; without it Neptune uses the pure-PyTorch FPS, which costs about +36% per training step and +89% at inference on real IceCube events (RTX 5090, batch 64).Validation
Trained on ~10.9M IceCube NuMu-CC events (direction reconstruction,
VonMisesFisher3DLoss) alongsideDynEdgeunder an identical schedule; Neptune reached a better best-validation loss. Tests and pre-commit hooks pass locally on CPU.🤖 Generated with Claude Code