Skip to content

Comments

Excluding hot pixels using percentile cut#11

Merged
jkbstepien merged 15 commits intomainfrom
@jkbstepien/hot-pixels
Jun 6, 2025
Merged

Excluding hot pixels using percentile cut#11
jkbstepien merged 15 commits intomainfrom
@jkbstepien/hot-pixels

Conversation

@jkbstepien
Copy link
Owner

@jkbstepien jkbstepien commented Jun 5, 2025

  • Excluding hot pixels using percentile cut
  • Setup tailwind in project
  • Setup typescript in project

Closes #9

@jkbstepien jkbstepien self-assigned this Jun 5, 2025
Copy link
Collaborator

@kacienk kacienk left a comment

Choose a reason for hiding this comment

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

remove dead files, besides LGTM

@grzanka grzanka requested a review from Copilot June 6, 2025 07:03
@grzanka
Copy link
Collaborator

grzanka commented Jun 6, 2025

@kacienk if you expect some changes to be done it the repo, then it doesn't make sense to approve PR. Any furher commit will invalidate the approval. You can say LGTM in the text.

image

This comment was marked as outdated.

@grzanka grzanka requested a review from Copilot June 6, 2025 08:34

This comment was marked as outdated.

@kacienk
Copy link
Collaborator

kacienk commented Jun 6, 2025

@kacienk if you expect some changes to be done it the repo, then it doesn't make sense to approve PR. Any furher commit will invalidate the approval. You can say LGTM in the text.

image

LGTM with changes means: "This PR needs some minor changes, that do not change logic in a fundamental manner. Once the changes are applied, then the PR is ready to merge." This way we avoid unnecessary re-review. I specifically described the changes that are needed and applying them would make this PR ready to merge.

By the way, from my experience, changes do not change the approval status on Github. My approval status didn't change.

@grzanka
Copy link
Collaborator

grzanka commented Jun 6, 2025

@kacienk if you expect some changes to be done it the repo, then it doesn't make sense to approve PR. Any furher commit will invalidate the approval. You can say LGTM in the text.
image

LGTM with changes means: "This PR needs some minor changes, that do not change logic in a fundamental manner. Once the changes are applied, then the PR is ready to merge." This way we avoid unnecessary re-review. I specifically described the changes that are needed and applying them would make this PR ready to merge.

By the way, from my experience, changes do not change the approval status on Github. My approval status didn't change.

You are correct, I though it's the other way around. This way your way of dealing with PR makes much more sense that what I suggest ! I am happy to learn that :)

@grzanka grzanka requested a review from Copilot June 6, 2025 10:15
Copy link
Contributor

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

This PR sets up Tailwind CSS and TypeScript support, and implements a hot-pixel exclusion feature for image conversion using percentile cut in the WASM backend and corresponding UI controls.

  • Configure project to use TypeScript and Tailwind CSS.
  • Add clip_pixels_with_percentiles in Rust WASM and new UI options for hot-pixel removal.
  • Refactor and remove legacy CSS and JSX files, migrate to Tailwind utility classes.

Reviewed Changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
client/vite.config.ts Added @ alias for source directory
client/tsconfig.json Introduced strict TypeScript compiler settings
client/tailwind.config.js Initialized Tailwind CSS content paths
client/src/components/images/ImageConverter.jsx Added hot-pixel removal UI and percentile inputs
api/src/lib.rs Implemented clip_pixels_with_percentiles WASM function
Comments suppressed due to low confidence (2)

client/src/components/images/ImageConverter.jsx:42

  • The variable name previesAspectRatios appears to be a typo and is unclear. Consider renaming to previewAspectRatio for clarity.
const [previesAspectRatios, setPreviesAspectRatios] = useState(16 / 10);

api/src/lib.rs:26

  • The new clip_pixels_with_percentiles function lacks dedicated unit tests. Adding tests for edge cases (e.g., empty data, extreme percentiles) would improve reliability.
pub fn clip_pixels_with_percentiles(

jkbstepien and others added 2 commits June 6, 2025 12:59
@jkbstepien jkbstepien merged commit 0b67429 into main Jun 6, 2025
2 checks passed
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.

excluding hot pixels by running percentile cut with user defined level

3 participants