-
Notifications
You must be signed in to change notification settings - Fork 1
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-41062: Add topic finding to efdUtils #64
Changes from 3 commits
d009768
ef8ab02
ce4f7b5
e46e971
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,6 +59,7 @@ | |
) | ||
from lsst.summit.utils.butlerUtils import removeDataProduct # noqa: F401 | ||
import lsst.daf.butler as dafButler | ||
from lsst.daf.butler import DatasetRef | ||
from lsst.resources import ResourcePath | ||
|
||
|
||
|
@@ -110,7 +111,7 @@ def setUp(self): | |
self.assertIsInstance(self.dataCoordMinimal, dafButler.dimensions.DataCoordinate) | ||
# NB the type check below is currently using a non-public API, but | ||
# at present there isn't a good alternative | ||
viewType = dafButler.core.dimensions._coordinate._DataCoordinateFullView | ||
viewType = dafButler.dimensions._coordinate._DataCoordinateFullView | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better to drop this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, thanks. |
||
self.assertIsInstance(self.dataCoordFullView, viewType) | ||
|
||
def test_getLatissDefaultCollections(self): | ||
|
@@ -178,14 +179,14 @@ def test_getMostRecentDataId(self): | |
|
||
def test_getDatasetRefForDataId(self): | ||
dRef = getDatasetRefForDataId(self.butler, 'raw', self.rawDataId) | ||
self.assertIsInstance(dRef, lsst.daf.butler.core.datasets.ref.DatasetRef) | ||
self.assertIsInstance(dRef, DatasetRef) | ||
|
||
dRef = getDatasetRefForDataId(self.butler, 'raw', self.rawDataIdNoDayObSeqNum) | ||
self.assertIsInstance(dRef, lsst.daf.butler.core.datasets.ref.DatasetRef) | ||
self.assertIsInstance(dRef, DatasetRef) | ||
dRef = getDatasetRefForDataId(self.butler, 'raw', self.dataCoordMinimal) | ||
self.assertIsInstance(dRef, lsst.daf.butler.core.datasets.ref.DatasetRef) | ||
self.assertIsInstance(dRef, DatasetRef) | ||
dRef = getDatasetRefForDataId(self.butler, 'raw', self.dataCoordFullView) | ||
self.assertIsInstance(dRef, lsst.daf.butler.core.datasets.ref.DatasetRef) | ||
self.assertIsInstance(dRef, DatasetRef) | ||
|
||
def test__dayobs_present(self): | ||
goods = [{'day_obs': 123}, {'exposure.day_obs': 234}, {'day_obs': 345, 'otherkey': -1}] | ||
|
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.
Now that with this new
findTopic
function, do you still want thegetSubTopics
function above?If yes, do you consider changing their names? Just by their names it's confusing what is for what. Might be even better just to have one function taking a regular expression.
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.
So I do think that we still need both - here is how I imagine using them:
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.
And I thought that the naming made the usage pretty clear, but maybe I am wrong. I don't like the idea of the regex, mainly because I wrote these to be easy, and as a self-declared regex-phobe, not only would I not be confident in how to best write that, I wouldn't even know how to use my own tools! 😄
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.
Given that, what do you think is best here?
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.
Naively I would be tempted to use a utility this way:
But I'm not a user of this and won't be in the short term, so I'd leave the judgement to you.
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.
(Actually that wasn't really regexp.. But you get my idea on wildcarding)
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.
Those exact examples now work 🙂