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

Cleanup before doc merge #124

Merged
merged 8 commits into from
Dec 19, 2023
Merged

Cleanup before doc merge #124

merged 8 commits into from
Dec 19, 2023

Conversation

nneveu
Copy link
Member

@nneveu nneveu commented Dec 18, 2023

Summary of Changes:

  • Moved older documentation to sphinx source location
  • Cleaned up formatting and imports
  • Added Sphinx to requirements.txt
  • Removed constants in data analysis directory
  • Removed uneeded directories in matlab2py dir

Notes

These small changes were made in preparation for the bigger documentation changes in PR #120

@nneveu
Copy link
Member Author

nneveu commented Dec 18, 2023

Of course I forgot to run the tests. Fixing those now.

@nneveu
Copy link
Member Author

nneveu commented Dec 18, 2023

Test are now cleaned up too.

Copy link
Collaborator

@eloiseyang eloiseyang left a 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.

docs/source/cor_plot.rst Show resolved Hide resolved
Copy link
Collaborator

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Collaborator

@phys-cgarnier phys-cgarnier left a 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.

@nneveu nneveu merged commit c432944 into main Dec 19, 2023
1 check passed
@nneveu nneveu deleted the cleanup branch December 19, 2023 01:37
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.

3 participants