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.

Copy link
Copy Markdown

Copilot AI left a comment

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
Copy link
Copy Markdown
Member Author

@arunkannawadi arunkannawadi 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 that kind of a PR where AI could be useful

Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/ci.yml Outdated
Copy link
Copy Markdown

Copilot AI commented Apr 30, 2026

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
Copy link
Copy Markdown
Contributor

sidneymau commented Apr 30, 2026

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.

Copy link
Copy Markdown
Contributor

@sidneymau sidneymau left a comment

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