Skip to content

Time Discretization - #87

Merged
Jacob-Chmura merged 17 commits into
mainfrom
dev/time_coarsening
Jul 3, 2025
Merged

Time Discretization#87
Jacob-Chmura merged 17 commits into
mainfrom
dev/time_coarsening

Conversation

@Jacob-Chmura

@Jacob-Chmura Jacob-Chmura commented Jul 2, 2025

Copy link
Copy Markdown
Member

Purpose

The purpose of this PR is to implement temporal coarsening. We expose a new method on the DGraph level which creates a deep-copy of the underlying storage using a coarsened time granularity.

Things to think about

  • Do we preserve the graph num_nodes and it's implicit semantics? If the result of my coarsening is such that certain nodes no longer exist, I currently keep them as part of the graph (this has implication for static node features shape, for instance). This does not technically violate any invariants but is different to our general workflow (where number of nodes matches max node id across all events on the graph)
  • We are setting up to basically clone the underlying storage on the downsampled graph. But if the user changes the storage backend they want, we'll still be constructing the one that the original graph had.
  • What happens if we coarsen a sliced graph? Currently, this creates the deep copy of the full storage. But, according to this API, the user could subgraph and then coarsen. In this case, the discretize() method in the backend needs to also take a DGSlice as an argument. And, this means we have to handle the edge case of empty graphs as well (since the sliced graph can end up empty).

Key Changes

  • DGStorage backend API has a new function for discretizing the storage
  • DGraph API has a new method that forwards to the storage to create a new coarsened graph
  • Add info to error message if user tries to use too coarse a granualrity when constructing the graph. They should discretize after construction.
  • def test_from_tgb_timestamp_remap_required_finer(
    this test method got improperly merged due to automatic conflict handling. This is fixed here.

TODO

  • confirm if we need the points of confusion above
  • add tests
  • consider refactor to make it easier to add reduction ops in the future

Relevant Prs

Close #86

Out of scope

  • vectorizing the discretization implementation using tensor diffs

@Jacob-Chmura Jacob-Chmura self-assigned this Jul 2, 2025
@codecov

codecov Bot commented Jul 2, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

Comment thread test/unit/test_data.py

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are patching issues that arised from incorrect auto-merge on previous PRs. This need not be reviewed.

Comment thread tgm/_storage/backends/array_backend.py
@shenyangHuang

Copy link
Copy Markdown
Collaborator
  • Do we preserve the graph num_nodes and it's implicit semantics? If the result of my coarsening is such that certain nodes no longer exist, I currently keep them as part of the graph (this has implication for static node features shape, for instance). This does not technically violate any invariants but is different to our general workflow (where number of nodes matches max node id across all events on the graph)

No matter how you coarsen, the number of nodes will never reduce. Theoretically this would be the case. Because when you coarsen, all you do is remove redundant edges which would originally have finer / different timestamps, now they are remapped to have the same timestamps. Therefore, if the node is in the graph then it will remain there.

@shenyangHuang

Copy link
Copy Markdown
Collaborator
  • We are setting up to basically clone the underlying storage on the downsampled graph. But if the user changes the storage backend they want, we'll still be constructing the one that the original graph had.

I thought the TGM assumption is that the user can't modify the graph once it is created (except for coarsening). I don't think we need to handle this issue. Once coarsened, the coarsen graph has no association with the original graph directly.

@shenyangHuang

Copy link
Copy Markdown
Collaborator
  • What happens if we coarsen a sliced graph? Currently, this creates the deep copy of the full storage. But, according to this API, the user could subgraph and then coarsen. In this case, the discretize() method in the backend needs to also take a DGSlice as an argument. And, this means we have to handle the edge case of empty graphs as well (since the sliced graph can end up empty).

actually this wouldn't exist if we only allow coarsen at construction haha

@Jacob-Chmura
Jacob-Chmura marked this pull request as ready for review July 3, 2025 13:48
@Jacob-Chmura Jacob-Chmura mentioned this pull request Jul 3, 2025
2 tasks
@Jacob-Chmura
Jacob-Chmura merged commit a7a294f into main Jul 3, 2025
6 checks passed
@Jacob-Chmura
Jacob-Chmura deleted the dev/time_coarsening branch July 3, 2025 17:49
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.

Discretize Operation on DGraph

2 participants