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

DM-43255: Update analysis code and refactor #93

Merged
merged 11 commits into from Mar 12, 2024
Merged

Conversation

mfisherlevine
Copy link
Contributor

No description provided.

Copy link

@taranu taranu left a comment

Choose a reason for hiding this comment

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

See specific comments.

@@ -50,6 +54,29 @@
"plotSourcesOnImage",
)

KNOWN_CAMERAS = ("regular", "wide", "fast")
Copy link

Choose a reason for hiding this comment

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

Not narrow? Shouldn't this be KNOWN_CAMERA_TYPES then, if you plan to have another camera with cameraType="regular"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think the KNOWN_CAMERAS needs renaming as they're physical cameras, not types, but yeah, this comment and all the others are absolutely right that I got part way through renaming regular to narrow but missed loads of places.

doSmoothPlot: bool


narrowCam = StarTrackerCamera("regular", "", "", True, 102, 5, 25, 0.95, 5, True)
Copy link

Choose a reason for hiding this comment

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

Again I find it slightly confusing to have regular/narrow but I assume there is a reason for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No no, you're right, this was also supposed to be renamed everywhere.

# streaming filenames are like GC103_O_20240304_000007_0001316.fits
# which is <camNum>_O_<dayObs>_<seqNum>_<streamSeqNum>.fits
# so 5 sections means streaming, 4 means normal
return len(os.path.basename(filename).split("_")) == 5
Copy link

Choose a reason for hiding this comment

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

You can also do os.path.basename(filename).count("_") == 5, FYI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, thanks, I do prefer that actually. Note the fence-post error there though - needs to be os.path.basename(filename).count("_") == 4 for the same behaviour!

sources = list(sourceDict.values())

allDayObs = set([s.dayObs for s in sources])
Copy link

Choose a reason for hiding this comment

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

You can drop the list [] since it'll work fine with a generator, FYI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thanks, I always forget that for some reason!


narrowCam = StarTrackerCamera("regular", "", "", True, 102, 5, 25, 0.95, 5, True)
wideCam = StarTrackerCamera("wide", "_wide", " wide", True, 101, 5, 25, 0.8, 5, True)
fastCam = StarTrackerCamera("fast", "_fast", " fast", True, 103, 2.5, 10, 0.95, 60, False)
Copy link

Choose a reason for hiding this comment

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

I strongly advise initializing dataclasses with kwargs (and ideally using kw_only=True) because chances are putting ten or more args in a row will bite you one day (and even if doesn't, it's hard to read).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, totally agree, and I usually do use that. I'm also not using slots here either, which is a really bad combination! This was because it was all growing very organically and in quite a panic a year ago when we first went on sky. Will tidy that up now 🙏

@mfisherlevine mfisherlevine merged commit bc198e8 into main Mar 12, 2024
4 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.

None yet

2 participants