-
Notifications
You must be signed in to change notification settings - Fork 0
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
DM-47777: Write interface for SOAR seeing data #50
base: main
Are you sure you want to change the base?
Conversation
966a91c
to
44969d7
Compare
44969d7
to
2cf7984
Compare
bd6e7a7
to
09e1bf5
Compare
09e1bf5
to
00d0325
Compare
600c1a5
to
9f7d659
Compare
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.
Mostly philosophical comments.
return None | ||
|
||
@staticmethod | ||
def adjustCoords(coords, height): |
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.
"Adjust" is super vague and any explanation of what this is doing would help.
See comment below about staticmethod.
results = self.reader.readtext(dateImage, detail=0) | ||
dateString = " ".join(results).strip() | ||
# replace common OCR errors | ||
dateString = dateString.replace(".", ":") |
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.
Clever. But for
loop here.
return astopyTime | ||
|
||
@staticmethod | ||
def thresholdImage(image, threshold): |
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.
All of these static methods are basically internal, so I would probably make them free functions and not export them.
def thresholdImage(image, threshold): | ||
return image.point(lambda x: 0 if x < threshold else 255, "L") | ||
|
||
def _preprocessImage(self, image): |
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.
A one sentence description will help the future reader.
imageBwNumpy = np.array(imageBw).astype(np.uint8) | ||
return imageBwNumpy | ||
|
||
def _getSeeingNumber(self, image) -> float: |
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.
This seems more like _ocrImage()
?
|
||
SOAR_IMAGE_URL = "http://www.ctio.noirlab.edu/~soarsitemon/soar_seeing_monitor.png" | ||
|
||
STORE_FILE = { |
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.
Hard coding this here seems like it's going to make this difficult to run in any other situation. I think it's better if these classes take filenames as parameters, and then the RA script specifies the full paths.
self._reload() | ||
return self.rowToSeeingConditions(self.df.iloc[-1]) | ||
|
||
def getSeeingForDataId(self, butler: Butler, dataId: DataCoordinate) -> SeeingConditions: |
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 have a philosophical disagreement with this and getSeeingForExpRecord
; this class doesn't need to know how to do butler things at all.
No description provided.