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

CutoutsFile and Measurements #302

Merged
merged 33 commits into from
Jun 28, 2024
Merged

Conversation

whohensee
Copy link
Collaborator

Change the structure of the cutouts and measurements such that we have a CutoutsFile that saves all the cutouts and takes up a single row in DB as FileOnDiskMixin, and Measurements which point to the CutoutsFile for relevant information.

See #217 and #272

@whohensee
Copy link
Collaborator Author

I made a slight change from what Guy and I discussed about the co_dict attribute of cutouts, but I tried to implement it in a way that seemed most intuitive to use. The main co_dict (which contains individual subdictionaries for cutout data under keys such as "source_index_0") is actually a class that inherits from dict, and when it is passed a key that it doesn't already have it will search on disk for that key. The end result is that the user of Cutouts can just use the co_dict attribute without worrying about whether or not the specific cutouts data is loaded, and a user of Measurements can just use the attributes (such as m.sub_data), and things will load if they exist on disk.

If I forgot a useful case, feel free to let me know.

@whohensee whohensee requested a review from guynir42 June 28, 2024 04:11
@guynir42 guynir42 marked this pull request as ready for review June 28, 2024 05:51
Copy link
Member

@guynir42 guynir42 left a comment

Choose a reason for hiding this comment

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

This is very good work. It indeed ended up touching a lot of code but it looks in very good shape.
I have a bunch of small comments and corrections, but nothing major.

Make sure your migration hash matches the latest main, and once you fix all the comments you can squash and merge.

models/cutouts.py Show resolved Hide resolved
models/cutouts.py Outdated Show resolved Hide resolved
models/measurements.py Outdated Show resolved Hide resolved
models/measurements.py Outdated Show resolved Hide resolved
models/measurements.py Outdated Show resolved Hide resolved
pipeline/measuring.py Outdated Show resolved Hide resolved
tests/models/test_cutouts.py Outdated Show resolved Hide resolved
tests/pipeline/test_measuring.py Show resolved Hide resolved
tests/pipeline/test_pipeline.py Outdated Show resolved Hide resolved
tests/pipeline/test_pipeline.py Outdated Show resolved Hide resolved
@guynir42
Copy link
Member

Please also try to run the models/test_image.py::test_image_products_are_deleted test and see if it is faster now that we've moved from list to single object. If so, mark it not skipped.

@whohensee whohensee merged commit 0cbe1ef into c3-time-domain:main Jun 28, 2024
7 checks passed
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