mkelley / pds3

Python module to read NASA Planetary Data System v3 files.
BSD 3-Clause "New" or "Revised" License
7 stars 6 forks source link

Error with missing units #2

Open saewitz opened 2 years ago

saewitz commented 2 years ago

Hi, thanks for this library! I ran into a few errors with missing units on a label data file from hirise, and added these lines:

from astropy import units
unit_meters_pixel = units.def_unit('meters/pixel', units.meter / units.pixel)
unit_local_day    = units.def_unit('localday/24', units.day / 24)
unit_bytes    = units.def_unit('bytes', units.byte)
units.add_enabled_units([unit_meters_pixel, unit_local_day, unit_bytes])

Reference to astropy defining units: https://docs.astropy.org/en/latest/units/combining_and_defining.html

I'm pretty sure unit_local_day is wrong, because it represents a 24-hour day according to the astropy documentation, but local day might be defined differently on labels. This could cause issues.

I also might be totally wrong that this needs to be added -- I'm new to this kind of data. If this is something that needs to be done, it might be good to add it to this library with fixes.

mkelley commented 2 years ago

Thanks for your interest. I'm glad you find it useful.

Yes, having a unit that varies based on context would be challenging to add. I'm thinking that I can define "localday" as a new IrreducibleUnit, and then one can convert to time using equivalencies:

>>> localday = u.def_unit('localday')                                                                                                          
>>> martian_time = u.Equivalency([(localday, u.s, lambda x: x * 88775.244, lambda x: x / 88775.244)])                                          
>>> (1 * localday).to('s', martian_time)                                                                                                       
Out[21]: <Quantity 88775.244 s>

Regarding the other units: I see that plural names are generally allowed by PDS3, so I'll see if I can sanitize the PDS3 the string before converting to astropy units.

mkelley commented 2 years ago

OK, I've made an update to address the units, but the bit mask isn't parsing. I'll need to work on that tomorrow.

saewitz commented 2 years ago

Cool, thank you! I ran into that issue as well, and just commented it out for now since I didn't need it.