Return the Dice overlap, name the CI artifacts per run, and say what actually failed - #879
Merged
Conversation
…ds keep their extension
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three things that made a quicktest run hard to read: the downloaded files could not be told apart,
a failure said only that something was out of range, and
dice_scoredid not return Dice.dice_scorereturns the Dice overlapFastSurferCNN.utils.metrics.dice_scorereturned the Dice dissimilarity, best 0, whileDiceScore.computein the same module returned the overlap, best 1. Its docstring claimed both, oneline apart. It now returns the overlap in both the scipy and the numpy branch.
Two call sites.
CerebNet/utils/meters.pywrites the result into a column labelled"Dice"and wasreporting
1 - Dice, so 0.02 appeared where 0.98 was meant; that is now correct. In the quicktestthe gate flipped from "close to 0" to "close to 1", which also makes
is_low_dicemean what itsays. The yaml tolerances keep their exact meaning, a distance from a perfect overlap, so no data
file changed and no verdict changed.
dice.csvfrom--collect_csvnow holds the overlap, so afile collected before this is not comparable to one collected after.
Failure messages
An assertion now names the worst offender and how far off it was, with the value and its limit in
the same units:
Dice scores in aseg.mgz are not within range! 7 of 41 labels below their minimum,
worst CC_Central at Dice 0.2263 (min 0.9900).
The stats tests gained the same, and they now separate a structure or measure that is missing from
the test subject from one whose values drifted, since the first is the larger failure:
The differences for some structures in aseg.stats exceed limits!
1 of 45 structures are missing from the test subject: Left-Lateral-Ventricle.
35 of 45 structures exceed their limit: Left-Inf-Lat-Vent, Left-Cerebellum-White-Matter, ...
A missing measure used to raise
KeyErrorwhile the message was being built rather than reportanything. The reported deviation is now relative to the same value the threshold is applied to, so
it can be read against the limit. The CSV collection also gained the zero guard the table path
already had, so two measures that are both 0 no longer divide by zero.
Artifact names
Every artifact carries the short commit sha and the subject, and the JUnit file is named after its
artifact, so results from several runs can sit in one download folder:
The subject is written in micrometres, zero padded, because the name must contain no dot: GitHub
serves an artifact as
<name>.zip, and Safari on macOS reads.8mm.zipas the extension, so a0.8mmartifact arrived unpacked as0.subject-idis unchanged, since it is also the subjectdirectory name and has to match the reference tarball. The new
artifact-idinput defaults tosubject-id, andrun-testspasses it on toload-processedso the download asks for the namethat was uploaded.
run-testsresolves the artifact name first and derives the JUnit path from it,which replaces its
junit-fileinput withjunit-dir.unittest.yamlfollows the same scheme anddrops from the full sha to eight characters.
Header comparison
test_image_headerscompares the NIfTI qform as a transform rather than field by field. A rotationhas two quaternion representations,
qand-q, and two headers writing the same geometry withopposite signs were reported as different. Headers with
qform_code0 describe no transform andtake their geometry from the sform, so their leftover fields are skipped too. MGH headers have no
qform and stay exactly compared.
Note for review
The workflows resolve
run-fastsurfer,run-testsandload-processedat@dev, so this PR's ownCI runs the previous versions of those actions and none of the naming changes are exercised here.
The artifact names need one look on the first
devrun after merge.