Skip to content

Compare reference images by default#16

Merged
arunkannawadi merged 1 commit into
mainfrom
ref_img_compare
May 4, 2026
Merged

Compare reference images by default#16
arunkannawadi merged 1 commit into
mainfrom
ref_img_compare

Conversation

@arunkannawadi

Copy link
Copy Markdown
Member

No description provided.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Updates the CI workflow so that reference image changes are detected during PRs, and attempts to generate visual comparisons and comment them back on the PR.

Changes:

  • Trigger the workflow on pull_request in addition to manual dispatch.
  • Detect whether the reference FITS image differs and gate subsequent steps on that result.
  • Add steps to generate PNGs via ds9, push updated reference files, and comment comparison images on the PR.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/ci.yml
Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/ci.yml Outdated

@arunkannawadi arunkannawadi left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is that kind of a PR where AI could be useful

Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/ci.yml Outdated

Copilot AI commented Apr 30, 2026

Copy link
Copy Markdown

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • ds9.si.edu
    • Triggering command: /home/REDACTED/work/_temp/ghcca-node/node/bin/node /home/REDACTED/work/_temp/ghcca-node/node/bin/node --enable-source-maps /home/REDACTED/work/_temp/copilot-developer-action-main/dist/index.js (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@sidneymau

sidneymau commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

Instead of installing ds9, I would suggest just using fits2bitmap (from astropy -- https://docs.astropy.org/en/stable/visualization/index.html#scripts)

@arunkannawadi

Copy link
Copy Markdown
Member Author

Oh I'm learning so many nifty fits tools that I didn't even know existed!

@arunkannawadi arunkannawadi marked this pull request as draft May 1, 2026 04:24
@arunkannawadi

Copy link
Copy Markdown
Member Author

OK, I give up on the approach where I wanted the CI workflow to push a comment with a before and after image for quick visualization. I'll rework this PR so the PNG version of the reference image also lives in the repo, and gets updated with the reference image gets updated. The before and after version will have to be from the PR diff.

@sidneymau

Copy link
Copy Markdown
Contributor

Trying to catch up on everything that was tried in here... I am happy to take a stab at getting the CI to post before/after images

@arunkannawadi

Copy link
Copy Markdown
Member Author

Okay, the workflow gets the job done. The difference can be seen in the Files changed. This is sufficient and in hindsight better than having multiple comments with the images. I'll clean up the PR before merging, but if you want to try something else that can be in another PR.

The only thing I dislike about using fits2bitmap is that it doesn't give a colorbar for the image

@arunkannawadi arunkannawadi marked this pull request as ready for review May 1, 2026 21:14
@sidneymau

Copy link
Copy Markdown
Contributor

@sidneymau

Copy link
Copy Markdown
Contributor

(possibly due to nobjects = 15?)

@arunkannawadi

Copy link
Copy Markdown
Member Author

It is not blank, just has fewer objects because I reduced it to 15 to trigger a difference and keep the run times shorter.

@sidneymau sidneymau left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me (I assume the plan is to revert to the full number of objects before merging?)

@arunkannawadi

Copy link
Copy Markdown
Member Author

Remove the temporary trigger commit, and the bot-generated commit in response.

@arunkannawadi arunkannawadi merged commit 7bf4702 into main May 4, 2026
@arunkannawadi arunkannawadi deleted the ref_img_compare branch May 4, 2026 17:59
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