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

Original frequency plot and removed periodic frequencies plot #37

Closed
wants to merge 16 commits into from

Conversation

p-pxmpo
Copy link
Contributor

@p-pxmpo p-pxmpo commented Nov 18, 2024

For this assignment all pull requests going forward should contain the following (please delete the placeholder text and add your own description)

Title

  • Original frequency plot and removed periodic frequencies plot

Linting

  • The code works with pylint

Contents

  • This PR contains the file that plots the frequency spectrum of the cube_grid_top_data.pkl file and plots the frequency spectrum with the high frequency components being removed using the periodic_erase function.
  • The file is in a common jupyter notebook called data_analysis.ipynb.

Labels and Milestones

  • Plot frequency information

Reviewer

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.

@p-pxmpo p-pxmpo added this to the plot frequency information milestone Nov 18, 2024
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@haiderabbas007
Copy link
Contributor

Could you please clarify this?
image

@SchrodingersStruggle
Copy link
Contributor

I can review this as requested.

@haiderabbas007
Copy link
Contributor

I can review this as requested.

Yes please, as I need this for my task.

@haiderabbas007
Copy link
Contributor

This as well
image

Copy link
Contributor

@SchrodingersStruggle SchrodingersStruggle left a comment

Choose a reason for hiding this comment

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

Averager and erase do not seem to be functions supported in the current requirements we have for the repository. You can either request the packages needed for these functions to be added to the requirements file, or change the functions into something which can be used by anyone.

@SchrodingersStruggle
Copy link
Contributor

I would reference specific lines, but I'm not entirely sure how to do that in a Jupyter file.

@p-pxmpo
Copy link
Contributor Author

p-pxmpo commented Nov 18, 2024

@SchrodingersStruggle Thank you for the review! those were functions defined in preperation.py in PR #30 but not merged into main by the time I created this PR. I will fix it today should be no later than 5PM.

@p-pxmpo
Copy link
Contributor Author

p-pxmpo commented Nov 18, 2024

@SchrodingersStruggle Thank you for the review! those were functions defined in preperation.py in PR #30 but not merged into main by the time I created this PR. I will fix it today should be no later than 5PM.

Actually cancel that I will fix it right now

@p-pxmpo
Copy link
Contributor Author

p-pxmpo commented Nov 18, 2024

@SchrodingersStruggle Thank you for the review! those were functions defined in preperation.py in PR #30 but not merged into main by the time I created this PR. I will fix it today should be no later than 5PM.

Actually cancel that I will fix it right now

Code has been fixed now, ready to be merged

Copy link
Contributor

@jkblc jkblc left a comment

Choose a reason for hiding this comment

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

The required filename is data_analysis.ipynb

@p-pxmpo
Copy link
Contributor Author

p-pxmpo commented Nov 18, 2024

The required filename is data_analysis.ipynb

Thank you, fixed now

@kylemasc917
Copy link
Contributor

Once one of the reviewers gives an approval ill merge

Copy link
Contributor

@jkblc jkblc 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

@p-pxmpo
Copy link
Contributor Author

p-pxmpo commented Nov 18, 2024

@SchrodingersStruggle awaiting your approval

Copy link
Contributor

@SchrodingersStruggle SchrodingersStruggle 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, especially now that dependancies are sorted.

@kylemasc917
Copy link
Contributor

You just need to resolve the last merge conflict and it should be ok to merge

@p-pxmpo
Copy link
Contributor Author

p-pxmpo commented Nov 19, 2024

@kylemasc917 merge conflict resolved

@p-pxmpo
Copy link
Contributor Author

p-pxmpo commented Nov 19, 2024

@kylemasc917 merge conflict resolved

As soon as this PR gets merged I will add my code with the original data_analysis.ipynb that was added with PR #36

@p-pxmpo
Copy link
Contributor Author

p-pxmpo commented Nov 19, 2024

@kylemasc917 check PR #44

@kylemasc917
Copy link
Contributor

ok feel free to close this

@p-pxmpo p-pxmpo closed this Nov 19, 2024
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.

5 participants