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 Notebooks DeadTime, Modeling,Window Functions #79

Conversation

AnonymousCodes911
Copy link
Contributor

-Fixed syntax errors/ value errors.
-ps.freq and ps.power had different lengths, so truncated one array to match the length.

-Fixed syntax errors/ value errors.
-ps.freq and ps.power had different lengths, so truncated one array to match the length.
satshed the notebooks (as suggested ) and edited the specific cells that were causing issues
@AnonymousCodes911
Copy link
Contributor Author

@matteobachetti,
I think this also closes #45 ?

Copy link
Member

@matteobachetti matteobachetti left a comment

Choose a reason for hiding this comment

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

Thanks @AnonymousCodes911!
A couple of points that need to be addressed:

  1. the window_functions notebook still has a lot of plot changes without significant code change, it should be cleaned up
  2. The modeling notebook should not need to filter a part of the array. If this is needed, it is a bug. I would avoid making this kind of change, which hides a possible bug and is not easily understandable from users reading the docs. Could you please eliminate this from the PR and open an Issue instead?
  3. Also, please do not explain your changes with comments in the notebooks, these messages should appear in the commit messages instead!

-removed obsolete seaborn dependencies
- reverted changes that might be a (?bug)
@AnonymousCodes911
Copy link
Contributor Author

@matteobachetti
sorry for the inconvenience, can you please check it again?
Thank you for your patience and understanding.

@matteobachetti
Copy link
Member

@AnonymousCodes911 Almost there! So, I currently see five files changed, and the Window one still has a lot of output (and you reverted the correct change 😅). Also, there still are comments that should not be there. Please check the current version of the "Files changed" tab (press "Refresh" if needed) to see all the changes you are proposing.

@AnonymousCodes911
Copy link
Contributor Author

AnonymousCodes911 commented Feb 16, 2024

about the Window_function file, locally on my desktop it shows only the error i wanted to fix, I dont understand why after committing, it piles up such a vast diff with changed images.

see this for reference:
image

i still see a large diff on the files changed tab, but the commit seems to work,
for reference:
image

again very sorry for creating so much confusion, let me know if we will want to squash the commits

@matteobachetti
Copy link
Member

matteobachetti commented Feb 17, 2024

@AnonymousCodes911 you have to make the diff with respect to our main, not your local copy. I suggest to clone the repo again in a separate directory, copy the window notebook, and make the small modification you are proposing in that one. I will squash and merge to eliminate all the intermediate steps.

Otherwise, it might be quicker and cleaner to:

  1. sync your fork's main with our current main, which has already slightly evolved since you started this PR.
  2. create a new branch and add in the changes you made one by one (one commit per notebook changed, explaining the modification in the commit message).

Please note that in the cross correlation notebook you fused two code lines:
from stingray import AutoCorrelationdt = 0.001 # seconds

@AnonymousCodes911
Copy link
Contributor Author

I created another PR😅, hopefully now the things will be in line, and it ll be much cleaner.

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.

2 participants