search

AI-SDC / SACRO-Viewer

A tool for fast, secure and effective output checking, which can work in any TRE.
Other
2 stars 1 forks source link

Integrate new format for per-cell ACRO results #145

Closed benbc closed 1 year ago

benbc commented 1 year ago

ACRO v0.3.0 adds a new coordinate (rather than mask-based) format for per-cell results. (See https://github.com/AI-SDC/ACRO/pull/104 and https://github.com/AI-SDC/ACRO/pull/114.)

Note that until this change, there has (in principle) been no way to combine per-cell results with multi-file outputs. We have some special-case code to accommodate this which will need to be changed.

The old format is still present as well, so we don't need to integrate the new format in order to take the release (note this is a correction to Tom's comment below). However it's possible that the old format will be removed at some point, so we should start using the new format instead.

madwort commented 1 year ago

we don't need to integrate this right now, but we will need to integrate next time we update from upstream.

bloodearnest commented 1 year ago

https://github.com/opensafely-core/sacro/pull/225

This parses the slightly awkward data structure in ACRO to a simpler to use one in the UI JSON.

Still needs the UI part, the cell highlighting code needs to index into the new "cell_index" object with ${x}{y} table coords.

lucyb commented 1 year ago

Is this still being worked on or has it been put to one side now?

ghickman commented 1 year ago

I wasn't planning to pick it back up now the project has wrapped

lucyb commented 1 year ago

@bloodearnest I know you were working on this. Did you get any further?

bloodearnest commented 1 year ago

No, I've been sidetracked with tech support

bloodearnest commented 1 year ago

This is done now with #251