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

Window/Unwindow #44

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Window/Unwindow #44

wants to merge 13 commits into from

Conversation

abruns123
Copy link

Linting

Have not tested linting yet. Will update

Contents

  • This pull request involves adding the window and unwindow function to the preparation.py file. It also contains some lambda
    functions which describe the windowing functions
  • My assignment: windowing / unwindowing with a selection of windows (1 member due Wednesday 2pm). The input must be a pandas time series.

Anyone may review this.

added window and unwindowing functionality
@abruns123 abruns123 added the python python related label Nov 6, 2024
@abruns123 abruns123 added this to the Window/Unwindow milestone Nov 6, 2024
@laserlab
Copy link
Contributor

laserlab commented Nov 6, 2024

@ubsuny/cp1-24 Who wants to review this? This should be reviewed and merged asap

@Cosmos491
Copy link

@laserlab I can review this

@laserlab laserlab requested a review from Cosmos491 November 6, 2024 21:04
@Cosmos491
Copy link

Here are some suggestions:

  1. Use def instead of lambda such as
    def hann_window(N, n):
    def blackman_window(N, n):
    def welch_window(N, n):
  2. replacing the N in def hann_window( window_size,n) as the variable name must be in snake case. Apply the change in N everywhere in your code.

Let me know if, @abruns123 , you run into any issue.

Adjusted my code to make it run correctly and fix pylint errors
Added test function for window/unwindow
@abruns123
Copy link
Author

Alright, I updated the pylint stuff and made sure to add unit test functions. Also included a few fixes where the code wasn't running as intended.

@laserlab
Copy link
Contributor

laserlab commented Nov 7, 2024

@Cosmos491 Please finish your review so this can get merged

@laserlab
Copy link
Contributor

laserlab commented Nov 7, 2024

@abruns123 Please resolve merge conflicts

@abruns123
Copy link
Author

merge conflicts should be resolved now

@Cosmos491
Copy link

There seems to be a syntax issue with the line starting with """test the powerspectrums length and type of export""". This could happen if the line is misplaced or if it is intended as a docstring but is not properly formatted. However, I also think that Instead of passing data as a parameter, define it within each test or use a pytest fixture to initialize shared data across tests.

@laserlab
Copy link
Contributor

laserlab commented Nov 7, 2024

@SchrodingersStruggle @abruns123 pytest issues have to be fixed before this can be merged

@abruns123
Copy link
Author

Okay, all the pytest errors should be fixed now

@laserlab
Copy link
Contributor

laserlab commented Nov 8, 2024

Okay, all the pytest errors should be fixed now

While the pytest is running now we gained a lot of inting error due to all the unnecessary triple quotes that were inserted. This has to be fixed.

@laserlab
Copy link
Contributor

laserlab commented Nov 8, 2024

@Cosmos491 you still didn’t approve or reject the PR …

@abruns123
Copy link
Author

I fixed the triple quotes. Some have to be there to make sure certain parts of the code that are in there which I didn't write aren't running as they produce pytest errors.

@Cosmos491
Copy link

The pytest seems to run fine, however there are some issues with pylint:

  1. The datacolumn is used without being defined. Make sure datacolumn is defined or replace it with the intended variable name.
  2. Rename type to avoid overwriting the built-in type function. For example, you could use window_type or another descriptive name.
  3. Ensure there’s no duplicate function definition at line 42. Review the function names and remove or rename the duplicate.
  4. Line 45 contains code that’s unreachable. This often happens if there’s a return or an exception before it. Remove or move unreachable code.
  5. Rename data within specific functions to avoid shadowing the outer data variable. For example, rename it to input_data within the function.
  6. Remove any unused imports like fft_powerspectrum, fft_mag, and get_timeseries if they are not used in test_preparation.py

@abruns123 Let me know if you need any clarification regarding the suggestions.

@laserlab
Copy link
Contributor

laserlab commented Nov 8, 2024

I fixed the triple quotes. Some have to be there to make sure certain parts of the code that are in there which I didn't write aren't running as they produce pytest errors.

You actually still have most tests commented out that were in there before that run without issues. That is not adding to files this is removing functionality and cannot be merged

@abruns123
Copy link
Author

But it was those sections of code that were causing the pytest errors. What am I supposed to do with them?

@laserlab
Copy link
Contributor

laserlab commented Nov 8, 2024

But it was those sections of code that were causing the pytest errors. What am I supposed to do with them?

the current test_preparation.py does not generate any pytest errors, so either you changed something that lead to failing or you are not using the most current version.

@abruns123
Copy link
Author

Alright, I copy and pasted in the most up to date pytest file. pytest check works. There are some pylint errors, but it should be mergeable now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python python related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants