Skip to content

feat: link data to graph image for description (DAVAI-38) #46

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

Merged
merged 7 commits into from
Jun 3, 2025
Merged
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 80 additions & 3 deletions src/models/assistant-model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,14 @@ const OpenAIType = types.custom({
},
});

interface IGraphAttrData {
legend?: Record<string, any>;
rightSplit?: Record<string, any>;
topSplit?: Record<string, any>;
xAxis?: Record<string, any>;
yAxis?: Record<string, any>;
}

/**
* AssistantModel encapsulates the AI assistant and its interactions with the user.
* It includes properties and methods for configuring the assistant, handling chat interactions, and maintaining the assistant's
Expand Down Expand Up @@ -62,9 +70,10 @@ export const AssistantModel = types
transcriptStore: ChatTranscriptModel
})
.volatile(() => ({
updateSonificationStoreAfterRun: false,
dataUri: "",
dataContextForGraph: null as IGraphAttrData | null,
uploadFileAfterRun: false,
dataUri: ""
updateSonificationStoreAfterRun: false,
}))
.actions((self) => ({
addDavaiMsg(msg: string) {
Expand Down Expand Up @@ -133,7 +142,6 @@ export const AssistantModel = types
}
});


const sendDataCtxChangeInfo = flow(function* (msg: string) {
try {
if (self.isLoadingResponse || self.isCancelling || self.isResetting) {
Expand Down Expand Up @@ -240,6 +248,7 @@ export const AssistantModel = types
yield sendFileMessage(fileId);
self.uploadFileAfterRun = false;
self.dataUri = "";
self.dataContextForGraph = null;
startRun();
} else {
const messages = yield self.apiConnection.beta.threads.messages.list(self.thread.id);
Expand Down Expand Up @@ -283,6 +292,67 @@ export const AssistantModel = types
}
});

const getAttributeData = flow(function* (graphID: string, attrID: string | null) {
if (!attrID) return { attributeData: null };

const response = yield Promise.resolve(codapInterface.sendRequest({
action: "get",
resource: `component[${graphID}].attribute[${attrID}]`
}));

const attributeData = response?.values
? {
id: response.values.id,
name: response.values.name,
values: response.values._categoryMap.__order
}
: null;

return { attributeData };

Choose a reason for hiding this comment

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

It's not clear why you want a one-element object here, rather than just returning attributeData directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bgoldowsky It made the definition for graphAttrData in getGraphAttrData cleaner. I guess it is a bit odd, though. In a new commit, I changed it to just return attributeData, and then in graphAttrData, I added the attributeData property name. I think it's kind of important to have that property name for each axis to make it absolutely clear to the LLM what the data is. There may be a way to do that more concisely, but I can't think of anything at the moment.

});

const getGraphAttrData = flow(function* (graphID) {
try {
const graph = yield getGraphByID(graphID);
if (graph) {
const legendAttrData = yield getAttributeData(graphID, graph.legendAttributeID);
const rightAttrData = yield getAttributeData(graphID, graph.rightSplitAttributeID);
const topAttrData = yield getAttributeData(graphID, graph.topSplitAttributeID);
const xAttrData = yield getAttributeData(graphID, graph.xAttributeID);
const yAttrData = yield getAttributeData(graphID, graph.yAttributeID);
const y2AttrData = yield getAttributeData(graphID, graph.yAttributeIDs ? graph.yAttributeIDs[1] : null);

Choose a reason for hiding this comment

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

Are you sure that graph.yAttributeIDs has two elements here?

Choose a reason for hiding this comment

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

Possibly you could use a map instead of the separate calls + combining step?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bgoldowsky Thanks for asking. In looking into this more, it seems what we actually want to do is const y2AttrData = yield getAttributeData(graphID, graph.y2AttributeID); and have separate yAxis and y2Axis properties on the returned object. I've made that change in the latest commit.


// Combine y-axis data if we have a second y-axis
const combinedYAxisData = y2AttrData.attributeData
? { attributeData: [yAttrData.attributeData, y2AttrData.attributeData] }
: yAttrData;

const graphAttrData = {
legend: legendAttrData,
rightSplit: rightAttrData,
topSplit: topAttrData,
xAxis: xAttrData,
yAxis: combinedYAxisData
};

if (graphAttrData) {
self.addDbgMsg("Data context for graph", formatJsonMessage(graphAttrData));
return graphAttrData;
} else {
self.addDbgMsg("No data context found for graph", formatJsonMessage(graph));
return null;
}
} else {
self.addDbgMsg("No graph found with ID", graphID);
return null;
}
} catch (err) {
console.error("Failed to get graph attribute data:", err);
self.addDbgMsg("Failed to get graph attribute data", formatJsonMessage(err));
return null;
}
});

const sendFileMessage = flow(function* (fileId) {
try {
const res = yield self.apiConnection.beta.threads.messages.create(self.thread.id, {
Expand All @@ -297,6 +367,10 @@ export const AssistantModel = types
image_file: {
file_id: fileId
}
},
{
type: "text",
text: `The following JSON data describes key aspects of the graph in the image. Use this context to improve your interpretation and explanation of the graph. ${JSON.stringify(self.dataContextForGraph)}`
}
]
});
Expand Down Expand Up @@ -336,6 +410,9 @@ export const AssistantModel = types
if (isImageSnapshotRequest) {
self.uploadFileAfterRun = true;
self.dataUri = res.values.exportDataUri;
const graphID = resource.match(/\[(\d+)\]/)?.[1];
// We also send data for the attributes on the graph for additional context
self.dataContextForGraph = yield getGraphAttrData(graphID);
}
// remove any exportDataUri value that exists since it can be large and we don't need to send it to the assistant
res = isImageSnapshotRequest
Expand Down