DM-28208: Add python access to readComponent() and associated Storable tests. #556
Conversation
erykoff
changed the title
erykoff
changed the title
|
For gen2, it's just not possible, and won't be. You have to do the read-an-exposure-with-a-single-pixel-bbox, and get it out of the |
| /// Read an arbitrary non-standard component by name. | ||
| std::shared_ptr<typehandling::Storable> readComponent(std::string const &componentName); |
There was a problem hiding this comment.
Should this be non-standard, or any component? Consider providing full API docs with e.g. parameter descriptions.
There was a problem hiding this comment.
Well, this is by string name, and the standard ones are by enumerated component, so I think this has to be non-standard. I certainly couldn't figure out the appropriate names to input to get the "standard" ones, which is why I put that comment.
There was a problem hiding this comment.
It's a shame because if readComponent could read any component it would simplify the ExposureFormatter code a lot.
There was a problem hiding this comment.
That seems like a lot more work here, because of passing enumerations up the chain, I think. But the readComponent(component) could also be exposed in python, I suppose.
There was a problem hiding this comment.
ExposureInfo exposes keys for looking up standard components, though because of my short-sightedness (i.e., assuming it was for internal use only) ArchiveReader::getComponent takes a string instead of a full Key. Assuming you don't change that, you'd need to call something like reader.readComponent(ExposureInfo::KEY_WCS.getId()) in C++ or reader.readComponent(ExposureInfo.KEY_WCS) in Python, but it certainly can be done.
There was a problem hiding this comment.
Not that it matters for your case, but having ExposureFitsReader::readComponent take a Key would make it more useful in C++; I'm happy to take that on as a separate ticket.
There was a problem hiding this comment.
👍 for making this take enums and handle standard components in another ticket. IIRC, at least some of the standard components are also unusual components here, at least in terms of how the affect they metadata reading and hence interact with each other, and really the only safe way to handle them here would be to intercept them and delegate to the special method for that component. And "intercept all standard components" feels like a case where you really want to be using enums somehow to make sure the check is exhaustive (and remains exhaustive even if new standard components are added in other parts of the codebase).
Edit: I think my memory of how those enums worked and where they should be used is just faulty. Please ignore the above; I don't think it actually made sense, and I'm happy that the rest of you have it under control.
| @@ -148,6 +153,69 @@ def testNewValue(self): | |||
| cppLib.assertCppValue(self.testbed, newstr) | |||
|
|
|||
|
|
|||
| @dataclass | |||
| class Blob(Storable): | |||
There was a problem hiding this comment.
What does this class add that DemoStorable, above, does not?
There was a problem hiding this comment.
That isn't persistable, afaict.
There was a problem hiding this comment.
Could it be made persistable? Or does Python-side support for Persistable require something (e.g., @dataclass) that's incompatible with the existing class?
There was a problem hiding this comment.
I suppose it can, but I also thought it was useful to have an example here that was a straight dataclass with yaml persistence...it's a bit redundant, is that a problem?
| /// Read an arbitrary non-standard component by name. | ||
| std::shared_ptr<typehandling::Storable> readComponent(std::string const &componentName); |
There was a problem hiding this comment.
👍 for making this take enums and handle standard components in another ticket. IIRC, at least some of the standard components are also unusual components here, at least in terms of how the affect they metadata reading and hence interact with each other, and really the only safe way to handle them here would be to intercept them and delegate to the special method for that component. And "intercept all standard components" feels like a case where you really want to be using enums somehow to make sure the check is exhaustive (and remains exhaustive even if new standard components are added in other parts of the codebase).
Edit: I think my memory of how those enums worked and where they should be used is just faulty. Please ignore the above; I don't think it actually made sense, and I'm happy that the rest of you have it under control.
No description provided.