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

Fixed - preprocess - actions #570

Merged
merged 12 commits into from
Jan 12, 2023
Merged

Fixed - preprocess - actions #570

merged 12 commits into from
Jan 12, 2023

Conversation

jatinkhilnani
Copy link
Contributor

Resolution for #477 - fix verified using preprocessor tutorial.

Copy link
Collaborator

@sammlapp sammlapp left a comment

Choose a reason for hiding this comment

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

We discussed that the desired changes should be made throughout the codebase before closing this issue or merging a branch.

One general thought after reviewing is that as a user, I would not be sure how to set random seeds to ensure reproducibility. Internally, several different packages generate random numbers, so the user would need to set random seeds for random, pandas, torch, and potentially numpy as well. I'm not sure what to do about this, but I'll create a separate issue for it.

opensoundscape/preprocess/actions.py Outdated Show resolved Hide resolved
@jatinkhilnani jatinkhilnani marked this pull request as draft November 16, 2022 18:31
@jatinkhilnani
Copy link
Contributor Author

We discussed that the desired changes should be made throughout the codebase before closing this issue or merging a branch.

One general thought after reviewing is that as a user, I would not be sure how to set random seeds to ensure reproducibility. Internally, several different packages generate random numbers, so the user would need to set random seeds for random, pandas, torch, and potentially numpy as well. I'm not sure what to do about this, but I'll create a separate issue for it.

The code to set seeds for all random methods across packages is a set of 4-5 statements that we can include in the documentation. Regarding the modifications, on second thought, instead of doing a sweep across the entire codebase, we could do it module wise with proper testing to ensure minimal risk. Have completed preprocess and annotations, will continue to do more before we do 0.8.0 release. The issue and branch could be kept open for working on other modules for subsequent releases.

@sammlapp
Copy link
Collaborator

If this work is complete, please change the PR from 'draft' to ready

@jatinkhilnani jatinkhilnani marked this pull request as ready for review January 12, 2023 12:58
@sammlapp sammlapp merged commit bbc99b9 into develop Jan 12, 2023
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.

Use pandas/native attributes instead of numpy methods when possible to process DataFrame/Series
2 participants