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

Refactor load_img #58

Merged
merged 12 commits into from
Mar 10, 2022
Merged

Refactor load_img #58

merged 12 commits into from
Mar 10, 2022

Conversation

emirkmo
Copy link
Member

@emirkmo emirkmo commented Mar 7, 2022

This is a full refactor of load_img to a more object oriented approach. All instruments are now classes that inherit from a generic Instrument parent/base class.

This now makes it much easier for me to add a few more fixes for image issues associated with edges. But before I do that, I wanted to merge a full working copy that reproduces the previous load_img functionality and makes it arguably easier to add new instruments.

The plan is to move most of this to an instruments.py file, and keep load_img.py a much smaller file that actually loads images. But that can be left to the future after the edge checking has also been implemented.

I will open some issues to track the other upcoming refactors.

Passing tests locally on python 3.7+ and a manual test of the run_photometry file. (Which we should also add as a unit test?)

Two caveats: we use spaces instead of tabs. This just made it much easier to read these multiline conditionals. I suggest we move to default 4 spaces following most major libraries and python black, as I would like to eventually enable it for automatic code formatting (to reduce code formatting maintenance overhead).

(I did a rebase onto devel before the merge to run the tests and run_photometry as flows table format had changed)

Fixes #29

@codecov
Copy link

codecov bot commented Mar 7, 2022

Codecov Report

Merging #58 (b0b6839) into devel (dfe9d28) will not change coverage.
The diff coverage is 0.00%.

❗ Current head b0b6839 differs from pull request most recent head 2ff032e. Consider uploading reports for the commit 2ff032e to get more accurate results

Impacted file tree graph

@@          Coverage Diff          @@
##           devel     #58   +/-   ##
=====================================
  Coverage   0.92%   0.92%           
=====================================
  Files         26      26           
  Lines       2281    2281           
=====================================
  Hits          21      21           
  Misses      2260    2260           
Impacted Files Coverage Δ
flows/load_image.py 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3ac086...2ff032e. Read the comment docs.

@emirkmo emirkmo merged commit b6dc652 into devel Mar 10, 2022
@emirkmo emirkmo linked an issue May 16, 2022 that may be closed by this pull request
@emirkmo emirkmo deleted the update_load_img branch August 9, 2022 10:40
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.

Refactor load_image Make it easier to add sites
1 participant