DM-24923: eliminate redundant class name & name requirement in butler command test cases #282
Conversation
There was a problem hiding this comment.
Minor comments. We are instantiating more Butler instances than before but I hope that no longer results in terrible overhead. I know that @kfindeisen had terrible performance on his computer with Butler taking seconds to be created so be aware this might not be a panacea.
tests/test_ingest.py
Outdated
|
|
||
| instrumentClassName = "lsst.obs.subaru.HyperSuprimeCam" | ||
|
|
||
| instrumentName = "HSC" |
tests/test_ingest.py
Outdated
| self.butler = Butler(self.root, run=self.outputRun) | ||
| self.visits = { | ||
| ingestDir = os.path.dirname(__file__) | ||
|
|
There was a problem hiding this comment.
I'm not sure I'm in favor of all the blank lines between properties.
|
|
||
| instrumentName = "HSC" | ||
|
|
||
| file = os.path.join(testDataDirectory, "hsc", "raw", "HSCA90402512.fits.gz") |
There was a problem hiding this comment.
This .gz reminds me that I think we want to change the default for ingest to also include .fits.gz and .fits.fz.
There was a problem hiding this comment.
@timj did you want me to do something about this in this ticket? (FWIW if it involves more than a line or 2 of code I'd rather ticket it)
There was a problem hiding this comment.
It was one line in python/lsst/obs/base/script/ingestRaws.py where we check file extension and could include the .gz/.fz there directly -- the ingestion clearly supports .fits.gz because the above file is a .fits.gz. What I'm now wondering is why we are specifying a file here at all when it seems like we could specify the directory -- maybe this is deliberate to ensure that we know exactly which dataIds we will end up with in the butler.
There was a problem hiding this comment.
We should take files as any number of extra positional arguments and let the shell do the globbing for us.
tests/test_ingest.py
Outdated
| @@ -69,7 +73,8 @@ def checkRepo(self, files=None): | |||
| # We ignore `files` because there's only one raw file in | |||
| # testdata_subaru, and we know it's a science frame. | |||
| # If we ever add more, this test will need to change. | |||
| expanded = self.butler.registry.expandDataId(self.dataIds[0]) | |||
| butler = Butler(self.root, run=self.outputRun) | |||
There was a problem hiding this comment.
I think this should be a readonly butler as above.
tests/test_ingest.py
Outdated
|
|
||
| @property | ||
| def visits(self): | ||
| butler = Butler(self.root, run=self.outputRun) |
There was a problem hiding this comment.
I think this should be:
butler = Butler(self.root, collections=[self.outputRun])because you want it to be read only and you aren't writing any data into it.
No description provided.