isi-vista / adam

Abduction to Demonstrate an Articulate Machine
MIT License
11 stars 3 forks source link

Stop merging objects #1206

Closed boyleconnor closed 1 year ago

boyleconnor commented 1 year ago

Fixes #1205.

Having just requested review, I realize I need to duplicate these changes in adam_preprocessing/single_file_extraction.py. That said, I think it is far enough along in shape_stroke_extraction.py to make review worth it

boyleconnor commented 1 year ago

As a check-in (just for the mask separation), how does this look:

Original RGB: rgb_0

STEGO output (not fixed): semantic_0

2 of the masks (there are 5 total, 2 are shadows and 1 is the background)

semantic_0.png_mask_2.png semantic_0 png_mask_2

semantic_0.png_mask_4.png semantic_0 png_mask_4?

spigo900 commented 1 year ago

@boyleconnor Those images look good to me. Re: names, now that I think of it we probably want to use os.path.splitext() to remove the .png before adding _mask_{i}.png but that's minor.

boyleconnor commented 1 year ago

I'm getting a lot of this warning: UserWarning: Graph is not fully connected, spectral embedding may not work as expected. I was worried this was due to the way I concatenate the stroke adjacency matrix, which of course leaves the strokes unconnected to one another. But it turns out the only call to SpectralClustering.fit() is before the stroke adjacency matrices are concatenated together (i.e. during the extract-from-each-file step). I'm not sure if this is something to worry about, but the stroke extraction doesn't look very good and I'm looking into why

spigo900 commented 1 year ago

Re: Spectral, I don't think it affects the stroke outputs. I've seen it a few times on the M5 objects curriculum as well. As far as I can tell the spectral clustering is only used as a way of regrouping strokes into objects, a different way from using the connected components of the graph. I think it's downstream of the bad strokes.

I expect the bad strokes are mostly a function of STEGO just picking up lots of spurious distinct object masks when run on real images. Turning stroke merging on might improve the stroke outputs a bit by fixing some of the disconnects and removing some of the bare patches. I expect it would at least decrease the number of warnings -- it looks like there are lots of disconnected strokes in the visualization, like you might expect given the number of masks.

boyleconnor commented 1 year ago

@spigo900 I think that piece of code is useful for extracting the concept_name (I believe it's used for GNN training) from the description.yaml. What do you think with it now?

spigo900 commented 1 year ago

@boyleconnor No, the GNN training repeats this process to extract a description of each situation. It shouldn't use the concept_name because that's where the GNN decode script stores the GNN output. If it looked for the label there, it would be easy to accidentally train the GNN on a previous GNN's labels rather than the ground truth. We actually want to use a dummy concept name here.

spigo900 commented 1 year ago

Specifically, the GNN code uses get_stroke_data() which includes this equivalent block of code. This is the tuple->label conversion code.

spigo900 commented 1 year ago

In particular explaining why I want to delete this: I just ran into a case with the color segmentation experiment where the GNN failed to overwrite the concept_name provided by stroke extraction, which meant ADAM was using ground truth labels as features. Because of this, the experiment results necessarily overestimated ADAM's accuracy. We don't need this if/else branch output, at all, so it seems better to just delete that code rather than put something in the concept_name slot that we could confuse for the GNN's output.

boyleconnor commented 1 year ago

@spigo900 there was a lot of extraneous code I ended up deleting in 9d97a40. Let me know if that's alright

spigo900 commented 1 year ago

@boyleconnor Looks okay to me.