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-43297: Update streaming sequence finder for new file layout #34
Conversation
data[seqNum] = [filename] | ||
else: | ||
data[seqNum].append(filename) | ||
if dayObs < 20240311: |
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 quick comment as to the significance of this date would be nice (even if just for future you?!)
else: | ||
data[seqNum].append(filename) | ||
else: | ||
# dirs here isn't the fully dirname, it's just the base dirname |
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.
"fully dirname"?!
So...call it baseDirs
?!
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.
Sure, I've gone for dirNames
and also improved the comment (which also contained a typo).
@@ -297,6 +298,7 @@ def findFastStarTrackerImageSources(filename, boxSize, attachCutouts=True): | |||
The sources in the image, sorted by rawFlux. | |||
""" | |||
exp = openFile(filename) | |||
expTime = exp.visitInfo.exposureTime # defaults to nan if not set |
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.
What does this evaluate to if it's "not set"? I.e. would it raise or is exposureTIme
guaranteed to be in exp.visitInfo
?
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 think it's guaranteed to be there (I think this is from our C++), and returns nan for all the cases I tested.
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.
Sorry, maybe the comment is ambiguous. What I meant was "if the upstream code hasn't set this, this attribute will be nan and so the expTime
will become nan in that case".
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.
Hopefully this all gets simpler once someone writes the metadata translator.
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.
Someone will need to write a repackager first...
@@ -81,7 +81,7 @@ def getStreamingSequences(dayObs): | |||
elif site == "summit": | |||
rootDataPath = "/project" | |||
else: | |||
raise ValueError(f"StarTracker data isn't available at {site}") | |||
raise ValueError(f"Finding StarTracker data isn't supported at {site}") |
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.
Does the added "Finding" really help?
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.
Well, it doesn't mean that the data isn't there, or that the data itself is unsupported (what would that even mean), just that the fancy auto-locate this data thing won't work.
# dirs here isn't the fully dirname, it's just the base dirname | ||
dirs = sorted(d for d in os.listdir(dataDir) if os.path.isdir(os.path.join(dataDir, d))) | ||
for d in dirs: | ||
files = sorted(glob.glob(os.path.join(dataDir, d, "*.fits"))) |
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'll take your word for it that you really want to list all the .fits
files 😀
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.
Consider using ResourcePath.findFileResources
to find them (with the advantage that it works on object stores as well).
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 do 🙂 Also, all this will be in the butler soon (I am told) so this is almost throw-away code (and that's also why it's in summit_extras)
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'm never going to look in S3 for this, and like you say, one day this will just be a butler.get for a single object.
DM-43297: Update streaming sequence finder for new file layout
No description provided.