Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for storing if a node is subsumed #16

Merged
merged 2 commits into from
Sep 12, 2024

Conversation

saulshanabrook
Copy link
Member

This pull request adds support for storing whether a node is subsumed or not. It introduces a new NodeData struct with a subsumed field, which can be used to track the subsumption status of a node.

@saulshanabrook
Copy link
Member Author

@mwillsey This is ready to review. I am using it in egraphs-good/egglog#424.

@mwillsey
Copy link
Member

Why not just put it on the Node itself?

@saulshanabrook
Copy link
Member Author

@mwillsey Sure I can do that. At first I wasn't sure if we wanted to allow arbitrary key/value data on each node to store any kind of metadata without having to change the schema. But at the moment we don't, so I guess if we find we need that we can always extend it to allow it.

@mwillsey
Copy link
Member

serde can also support that arbitrary metadata approach if your prefer that https://serde.rs/attr-flatten.html#capture-additional-fields

@saulshanabrook
Copy link
Member Author

saulshanabrook commented Sep 12, 2024

I was thinking that the metadata might want to be in its on key in the JSON, only because that's the way I had seen it work in the Jupyter Notebook format. Any cell can have arbitrary metadata stored in the metadata field.

{
    "cell_type": "code",
    "execution_count": 1,  # integer or null
    "metadata": {
        "collapsed": True,  # whether the output of the cell is collapsed
        "scrolled": False,  # any of true, false or "auto"
    },
    "source": "[some multi-line code]",
    "outputs": [
        {
            # list of output dicts (described below)
            "output_type": "stream",
            # ...
        }
    ],
}

But I can't say I feel strongly that's the right thing to do here (if we expect various e-graph libraries to want to store other metadata on nodes without first updating this repo). If we did, one consumer of that metadata could be the new interactive visualizer, letting you see those arbitrary key value pairs when you select a node.

But I can also see just sticking with only our known fields for know and highlighting those in the visualizer (like by making subsumed nodes lighter color or something), and only switching to a more general schema when the need arises. So I am happy keeping it on the node as you suggested.

@mwillsey mwillsey merged commit 3625e0d into egraphs-good:main Sep 12, 2024
2 checks passed
@saulshanabrook saulshanabrook deleted the subsume-data branch September 12, 2024 17:06
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