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.

Sign up for GitHub

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

Jump to bottom

Save out detector artifacts on detector training #224

Merged
merged 3 commits into from
Dec 6, 2024
Merged

Save out detector artifacts on detector training #224

merged 3 commits into from
Dec 6, 2024

Conversation

ksikka
Copy link
Collaborator

@ksikka ksikka commented Nov 26, 2024 •
edited
Loading

It's a lot of diffs, 1. because I moved some things around.

  • export_predictions_and_labeled_video method moved from scripts.py to predictions.py.
  • get_context_img_paths moved to io.py
  • regenerated api docs.
  1. Because of the tests. I checked in some input files, and the cropzoom expected output files. This way we can visually see the cropping. If we make any intentional changes, all we have to do is copy the test output to the expected_model_output directory.

@ksikka ksikka marked this pull request as draft November 26, 2024 00:16
@ksikka ksikka force-pushed the detector_outputs_3 branch 7 times, most recently from d9cacef to fdcb4bc Compare December 2, 2024 20:14
ksikka
ksikka commented Dec 2, 2024
View reviewed changes
setup.py Show resolved Hide resolved
@ksikka ksikka changed the title [WIP] Save out detector artifacts Save out detector artifacts Dec 2, 2024
@ksikka ksikka marked this pull request as ready for review December 2, 2024 20:29
@ksikka ksikka changed the title Save out detector artifacts Save out detector artifacts on detector training Dec 2, 2024
@ksikka ksikka force-pushed the detector_outputs_3 branch from fdcb4bc to 8b1a08d Compare December 4, 2024 17:42
ksikka
ksikka commented Dec 4, 2024
View reviewed changes
.gitattributes Outdated Show resolved Hide resolved
@ksikka ksikka force-pushed the detector_outputs_3 branch from 8b1a08d to a74e015 Compare December 4, 2024 17:57
ksikka
ksikka commented Dec 4, 2024
View reviewed changes
scripts/predict_new_vids.py Outdated Show resolved Hide resolved
themattinthehatt
themattinthehatt requested changes Dec 5, 2024
View reviewed changes
Copy link
Collaborator

@themattinthehatt themattinthehatt left a comment

Choose a reason for hiding this comment

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

Looks like Git LFS still keeps track of LFS diffs :/
Main comment is that we should try using Figshare to store the test files so that the repo diffs don't become bloated over time
https://stackoverflow.com/questions/59383255/git-lfs-still-diff-ing-files

Other minor comment is to run isort before next push

lightning_pose/train.py Show resolved Hide resolved
lightning_pose/utils/cropzoom.py Outdated Show resolved Hide resolved
lightning_pose/utils/cropzoom.py Show resolved Hide resolved
lightning_pose/utils/cropzoom.py Outdated Show resolved Hide resolved
tests/utils/test_cropzoom.py Show resolved Hide resolved
tests/utils/test_cropzoom.py Show resolved Hide resolved
@ksikka ksikka force-pushed the detector_outputs_3 branch 2 times, most recently from 0313f15 to 954053f Compare December 6, 2024 17:45
@ksikka ksikka force-pushed the detector_outputs_3 branch from 954053f to 836315a Compare December 6, 2024 17:49
@ksikka
Copy link
Collaborator Author

ksikka commented Dec 6, 2024

Addressed all comments! We are off git-lfs, onto figshare.

themattinthehatt
themattinthehatt approved these changes Dec 6, 2024
View reviewed changes
lightning_pose/utils/cropzoom.py Show resolved Hide resolved
lightning_pose/utils/io.py
@@ -11,7 +14,6 @@
__all__ = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

add "get_context_img_paths"

@themattinthehatt themattinthehatt merged commit 1f79a73 into paninski-lab:main Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@themattinthehatt themattinthehatt themattinthehatt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ksikka @themattinthehatt