DM-22073: Add support for writing matplotlib figures #205
Conversation
tests/test_matplotlibFormatter.py
Outdated
| import os | ||
| import shutil | ||
|
|
||
| import matplotlib |
There was a problem hiding this comment.
Please protect the matplotlib import and skip the tests if matplotlib is not available. daf_butler should not require matplotlib.
There was a problem hiding this comment.
Is this resolved? I agree that it's important.
There was a problem hiding this comment.
(It looks resolved, given the new try block for the import of matplotlib, but I haven't been able to test it in practice.)
There was a problem hiding this comment.
It's hard for any of us to test it, I think, because we all use conda enviroments that always need to have matplotlib for other reasons. But I'm pretty confident what was merged will work.
| """Interface for writing matplotlib figures. | ||
| """ | ||
|
|
||
| extension = '.png' |
There was a problem hiding this comment.
Please use double quotes in daf_butler to be consistent with the rest of the code base.
tests/test_matplotlibFormatter.py
Outdated
| universe=butler.registry.dimensions) | ||
| butler.registry.registerDatasetType(datasetType) | ||
| pyplot.imshow(np.random.randn(3, 4)) | ||
| butler.put(pyplot.gcf(), datasetType) |
There was a problem hiding this comment.
@TallJimbo does this work without a DataId because the dataset type has no dimensions?
There was a problem hiding this comment.
I think that since this code could be seen as providing example usage that we should not be relying on the super special case here of datasetType and datasetRef being interchangeable. Please receive the ref from the butler.put here and then use it later in the getUri and later butler methods below. This all works because there are no dimensions but we don't want to give the impression that it's going to work more generally.
ref = butler.put(...)
parsed = ...(butler.getUri(ref))
...
butler.datasetExists(ref)etc.
tests/test_matplotlibFormatter.py
Outdated
| # predicting the filename path based on test run | ||
| self.assertTrue( | ||
| filecmp.cmp( | ||
| os.path.join(self.root, 'testrun', datasetType.name, |
There was a problem hiding this comment.
I think I would prefer it if this path was constructed from calling butler.getUri since then you aren't relying on knowing how the file name was constructed within the datastore.
There was a problem hiding this comment.
That's what we tried first, but annoyingly filecmp apparently can't handle file:// prefixes, and it occurred to me that actual usage would look more like this, but perhaps with users explicitly defining the template so they'd know where to look in general, rather than asking the butler where to look each time.
Do you have an incantation handy for how to get that file:// prefix off gracefully?
There was a problem hiding this comment.
You can either use ButlerURI or you can go for the standard option of:
parsed = urllib.parse.urlparse(uri)and then use parsed.path.
| file.name, | ||
| shallow=True | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Please add calls to butler.datasetExists and butler.remove so that we can check that these PNG images are being treated like a normal dataset. Also try to do a butler.get and check that it fails with with self.assertRaises.
|
@leeskelvin, I'm going to let you take the lead on addressing these comments, but please find me for more pair-programming if you want help with anything. |
leeskelvin
force-pushed
the
tickets/DM-22073
branch
7 times, most recently
from
7c004c5
to
06755d1
Compare
leeskelvin
force-pushed
the
tickets/DM-22073
branch
from
06755d1
to
8cb7c8f
Compare
No description provided.