Expose structs: libraw_imgother_t and libraw_lensinfo_t#289
Expose structs: libraw_imgother_t and libraw_lensinfo_t#289pam-param-pam wants to merge 6 commits intoletmaik:mainfrom
libraw_imgother_t and libraw_lensinfo_t#289Conversation
letmaik
left a comment
letmaik
left a comment
There was a problem hiding this comment.
Thanks for the contribution, I think this is quite useful, especially if other libraries don't cover it. It definitely needs some tests, and I would change it to typed named tuples. Bonus points if you can extend the sphinx API docs as well.
| return { | ||
| "min_focal": l.MinFocal, | ||
| "max_focal": l.MaxFocal, | ||
| "max_ap_min_focal": l.MaxAp4MinFocal, | ||
| "max_ap_max_focal": l.MaxAp4MaxFocal, | ||
| "lens_make": l.LensMake.decode('utf-8', 'ignore'), | ||
| "lens": l.Lens.decode('utf-8', 'ignore'), | ||
| "focal_35mm": l.FocalLengthIn35mmFormat, | ||
| "exif_max_ap": l.EXIF_MaxAp, | ||
| } |
There was a problem hiding this comment.
Which of those fields do you need yourself? If the goal is lens correction, then probably only the make and the name. I would start small and only expose what people actually need for now.
There was a problem hiding this comment.
I removed uneeded fields. I left maker, model, min and max focal
| "shutter": o.shutter, | ||
| "aperture": o.aperture, | ||
| "focal_len": o.focal_len, | ||
| "timestamp": o.timestamp, |
There was a problem hiding this comment.
What data type is this? Does it need to be converted to a python datatime?
There was a problem hiding this comment.
I converted it into a python datetime. Idk if the timezones are correct
| "iso_speed": o.iso_speed, | ||
| "shutter": o.shutter, | ||
| "aperture": o.aperture, | ||
| "focal_len": o.focal_len, |
There was a problem hiding this comment.
Can you rename this to focal_length?
|
|
||
| return { | ||
| "iso_speed": o.iso_speed, | ||
| "shutter": o.shutter, |
There was a problem hiding this comment.
Let's rename this to shutter_speed.
| "max_focal": l.MaxFocal, | ||
| "max_ap_min_focal": l.MaxAp4MinFocal, | ||
| "max_ap_max_focal": l.MaxAp4MaxFocal, | ||
| "lens_make": l.LensMake.decode('utf-8', 'ignore'), |
There was a problem hiding this comment.
Just a small nit: this is "make" in the Exif spec (and as such in exiftool, exiv2, etc.)
For the camera body it is already "make" in libraw_iparams_t...
| "max_ap_min_focal": l.MaxAp4MinFocal, | ||
| "max_ap_max_focal": l.MaxAp4MaxFocal, | ||
| "lens_make": l.LensMake.decode('utf-8', 'ignore'), | ||
| "lens": l.Lens.decode('utf-8', 'ignore'), |
| self.ensure_unpack() | ||
| cdef libraw_lensinfo_t *l = &self.p.imgdata.lens | ||
|
|
||
| return { |
There was a problem hiding this comment.
I think it would be better to expose this as named tuples like for the sizes property. Ideally typed like here: https://typing.python.org/en/latest/spec/namedtuples.html
|
I renamed fields. I also added 2 tests. But im unsure if they are correct. I don't really see the point of veryfing each field since its either fully correct or not. And even if there was a bug it would be due to libraw and not due to the rawpy
I would also like to expose class GPS(NamedTuple):
latitude: float
longitude: float
altitude: float | None
altitude_ref: int
timestamp: str | NoneAnd idk if u want the the wrapper to handle the conversion(and if yes, in |
|
Any news? |
3 participants
Per what i said in #287
I exposed imgother_t and lensinfo_t structs.
Its my first real PR to an open source project. And I have no expierience with c++ pyhon wrappers so i apologize if I did something wrong.
Im unsure whetever i should add tests for this.
And im unsure if i should update the docs/add examples.
Im looking forward to hear what i should change/update