-
Notifications
You must be signed in to change notification settings - Fork 21
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
Cleanup before doc merge #124
Conversation
Of course I forgot to run the tests. Fixing those now. |
Test are now cleaned up too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate how you simplified the file structure here. Thank you for getting a start on translating Matlab to Python, I think this will be a really good tool for the HLA team. I just left one minor comment on the naming convention cor_plot
vs corr_plot
, but I feel these changes could be merged as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that this file was removed, is the plan not to add it in the data analysis folder or are we deprecating it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to deprecate it. This test file mostly has bare bones test set up/template and two bucket tests. I'm not sure these fit into the new structure/image code that was written. This is a good comment though, I'm going to go back and double check the original file to see if there are any functions we should keep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea after a second look, I still want to remove these. Some of the functions are in your new fitting tool, some can be replaced with functions in scipy (peak finding), the rest are specific to some image processing tricks. We can always look at this on old hashes/commit if we decide we want a piece of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I just wanted to verify that those deleted files are no longer needed, everything else looks good.
Summary of Changes:
Notes
These small changes were made in preparation for the bigger documentation changes in PR #120