Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clearing Plot for Cube Side #41

Merged
merged 3 commits into from
Nov 19, 2024

Conversation

SchrodingersStruggle
Copy link
Contributor

@SchrodingersStruggle SchrodingersStruggle commented Nov 18, 2024

Clearing Plot for Cube Side

Contents

  • Removes periodicity from Cube Side View photo for a more clear image.

Reviewer

  • Open

Any further questions on formatting pull requests can be asked in the issues tab or on discord.
This document can be changed at any time.

@SchrodingersStruggle SchrodingersStruggle added the Algorithm/subalgorithm in relation to the fulfillment of a full functioning code label Nov 18, 2024
@SchrodingersStruggle SchrodingersStruggle added this to the Cleaned-up Image milestone Nov 18, 2024
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@SchrodingersStruggle SchrodingersStruggle changed the title initial commit Clearing Plot for Cube Side Nov 18, 2024
rendering. Added windowing to plot.
@SchrodingersStruggle
Copy link
Contributor Author

For some reason even after lowering dpi the file is not rendering all of the parts of the file aren't able to be viewed when clicking view in GitHub. There is a section underneath which displays the cleaned up fft plot, and then does an ifft to show the cleaned up image. Downloading the file will hopefully allow it to be viewed for whoever reviews this.

@kylemasc917
Copy link
Contributor

@haiderabbas007 Could you please review this asap?

Copy link
Contributor

@haiderabbas007 haiderabbas007 left a comment

Choose a reason for hiding this comment

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

My 2 ¢:

Functionally and operationally the code is fine. It is logically organized into data loading, visualization, FFT processing, cleanup, and final visualization steps. The use of a median threshold and central masking for filtering frequency components is clear and efficient. High DPI and detailed titles, labels, and color maps enhance interpretability.

There are a few things though that may improve its aesthetic beauty:

  • Encapsulate repetitive operations like plotting and filtering into functions for better reusability and readability, e.g., a plot_image function for visualizations or a filter_frequency_data function.
  • Instead of hardcoding mask_size = 25, allow it to be an adjustable parameter passed to the cleanup function.
  • Use dynamic filenames or command-line arguments for the file_path.
  • Add docstrings or comments explaining why specific thresholds (e.g., threshold = np.median(...) * 5) and windowing (e.g., Hanning) are chosen.
  • Add checks for file existence and data integrity before processing.
  • Verify the dimensionality of image_data to ensure compatibility with grayscale conversion.
  • Overlay key frequencies on the magnitude spectrum plots for better interpretability.
  • Explore different window functions (e.g., Blackman or Kaiser) and compare their effects on the results.

Out of curiosity, what is the structure of the raw data (raw_data.iloc[0, 0])? It would help to see a brief description or shape validation to ensure robustness.

I am approving this PR as there are no functionality issues (and it's time for HW 7). The images don't fully load on GitHub, but they do load when opened separately, so it's GitHub's fault, not yours.

@haiderabbas007
Copy link
Contributor

@haiderabbas007 Could you please review this asap?

done

@kylemasc917 kylemasc917 merged commit 70183bb into ubsuny:main Nov 19, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Algorithm/subalgorithm in relation to the fulfillment of a full functioning code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants