Skip to content

Conversation

@DeannaLC
Copy link
Member

Resolves: (#1309)

  • I passed the docker hub test, and images can be built successfully.
  • I passed the GitHub CI test, so rodan functionalities and jobs work.

Added an optional RGB PNG input and output to Column Splitting so the split image can be passed into the Interactive Classifier.

@homework36
Copy link
Contributor

homework36 commented Jul 7, 2025

Just a few things:

  1. Why do we need an RGB input/output?
    Could the images be converted to RGBA offline before passing them into Rodan? That might be a simpler and more consistent approach.
  2. If keeping RGB is necessary, please clarify the rationale—ideally by linking to a related issue or briefly explaining the use case in the comments. This will help others understand why we’re introducing a new input/output instead of reusing existing ones like "Layer 4" or "Layer 5". A more descriptive name would also be helpful.
  3. Minor suggestion:
    It might be worth double-checking that get_stacked_image handles RGB images correctly. Here's the relevant code for reference:
    def get_stacked_image(img,ranges):
    chunks = []
    max = 0
    # add each column to a list
    # also find the widest column. All other columns
    # must be padded to this width
    for r in ranges:
    chunks.append(img[:,r[0]:r[1]])
    if chunks[-1].shape[1] > max:
    max = chunks[-1].shape[1]
    # pad each column to the widest column
    output = []
    for chunk in chunks:
    output.append(np.pad(chunk,((0,0),(0,max-chunk.shape[1]),(0,0)),mode='constant',constant_values=255))
    # stack and return
    return np.vstack(output)

@DeannaLC
Copy link
Member Author

DeannaLC commented Jul 9, 2025

The main use case for the change is the Interactive Classifier section of the E2E workflow:
image

I wrote a little more in issue #1309 about why the split image is needed in the IC. Using the other layer inputs and outputs also works correctly, but this would just remove the overhead of having to convert to PNG RGBA before running the job. I've tested this locally with a couple RGB image inputs which have worked correctly, so it doesn't look like any changes need to made to get_stacked_image.

Workflow Inputs:
Column Splitting Test Files.zip

019v Split Image Output:
019v Column Splitting - Image.zip

044v Split Image Output:
044v Column Splitting - Image.zip

@martha-thomae martha-thomae requested a review from homework36 July 9, 2025 15:13
@martha-thomae
Copy link

@homework36, is this ready to be merged then? Would you mind giving it a review and approving it if everything seems in order? Thank you!

@homework36
Copy link
Contributor

@DeannaLC Thank you for the updates! I can confirm that this is working fine for image 044v on my local machine as well. I was not able to test 019v because I didn't find the original image in the test files.
I wrote a few things, but what you really need to modify for this PR is the first one, and it should be very quick.

  • Like I said in the previous comment, can you rename your new input (and also output) port name to something like RGB Image? It will be confusing and too complicated to have multiple layers and then Image at the same time.
  • To justify that get_stacked_image works properly for RGB images as well, what we expect is not only that "it works for a couple RGB image inputs" but also something like "based on this code, it doesn't matter if the input is RGB or RGBA because the function operates generically on array dimensions without assuming a specific channel count, and the padding with constant_values=255 produces white pixels for both RGB (255,255,255) and RGBA (255,255,255,255) formats" so that we make sure it does not work by chance.
  • Do we not have jobs to convert between RGB and RGBA images?

@DeannaLC
Copy link
Member Author

DeannaLC commented Jul 12, 2025

Sorry, here's the 019v image if you still need it: 019v.zip

I just pushed a couple commits for the changed port name and a little comment about get_stacked_images working for both 3 and 4 channels - it's exactly how you explained that constant_values makes tuples that conform to the 3 or 4 channels.

I did however realize there were issues with the All Layers output which caused the job to fail because of the difference in 3 vs 4 channels so I just excluded the image from it, but let me know if I should do it differently.

There are no jobs for converting from RGB to RGBA currently.

@homework36
Copy link
Contributor

I tested 019v and it works as expected. I also tested what if the column split job has no RGB Image input port, but with the RGB Image output. It fails properly with RuntimeError: The job did not produce the output file for RGB Image.
Good catch on All Layers output @DeannaLC. To be honest, I'm not sure when we will need this output and what it is for. I guess it is just to provide a preview of what column split does overall.
Without the RGB image input, the All Layers for 019v looks like this

But we can also convert this RGB layer into RGBA and output something like this
Screenshot 2025-07-14 at 11 32 47 AM
The image is huge, so I only inserted a small screenshot. Here's the original output image, and it clearly displays the stacked layers on top of the RGB image.
all layers output 019v.png.zip
To do this, we only need to modify the newly added lines 118-119 to be

if key != 'RGB Image':
    layers.append(img)
 else:
    if img.shape[2] == 3:  # If no alpha channel
        img = cv.cvtColor(img, cv.COLOR_BGR2BGRA) # Add alpha channel with default 255 (not transparent)
        layers.append(img)

This seems to me more of a design choice, so maybe @martha-thomae can take a look and decide?

@homework36
Copy link
Contributor

And everything else looks good to me.

@martha-thomae
Copy link

Good catch on All Layers output @DeannaLC. To be honest, I'm not sure when we will need this output and what it is for. I guess it is just to provide a preview of what column split does overall.
Without the RGB image input, the All Layers for 019v looks like this ...
This seems to me more of a design choice, so maybe @martha-thomae can take a look and decide?

I would leave the All Layers to only include the layer-related information, so no split RGB image (at least for now). Thanks for the question, @homework36! And good catch, @DeannaLC!

Copy link
Contributor

@homework36 homework36 left a comment

Choose a reason for hiding this comment

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

This is all good now.

@DeannaLC DeannaLC merged commit 700a4d6 into develop Jul 16, 2025
2 checks passed
@github-actions github-actions bot mentioned this pull request Jul 16, 2025
2 tasks
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.

4 participants