Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify orbital_parameters metadata #922

Closed
djhoese opened this issue Oct 3, 2019 · 3 comments · Fixed by #950
Closed

Clarify orbital_parameters metadata #922

djhoese opened this issue Oct 3, 2019 · 3 comments · Fixed by #950

Comments

@djhoese
Copy link
Member

djhoese commented Oct 3, 2019

Currently the orbital_parameters and what values are likely to be found in this channel metadata are defined in the readers documentation here. However, there is no mention of orbital_parameters in the custom reader documentation. The current docs are also unclear about what units some of these values are in. Most importantly altitude needs to be in kilometers (why?!?!?) for the angle calculations in pyorbital and needs to be relative to the surface of the earth. Docs currently say:

Current position of the satellite at the time of observation in geodetic coordinates (i.e. altitude is normal to the surface).

The word normal to me says "perpendicular" to the surface, but does not necessarily mean relative to the surface.

@sfinkens
Copy link
Member

sfinkens commented Oct 4, 2019

@djhoese I think this was one of the reasons why @mraspaud proposed to attach units to these quantities. I remember that unyt was an option on the table, but that was a couple of months ago. Until this is implemented, we should mention the required units in the reader docs.

I will also clarify the wording (I think "relative and normal to the surface" should do?) and link to the reader:metadata conventions from the custom reader docs.

@mraspaud
Copy link
Member

mraspaud commented Oct 4, 2019

two quick comments:

  • we should use the same units for all satellites until we have the unit carried with the quantity
  • I started using pint in the ninjotiff writer, as it's compatible with xarray.DataArrays (unyt isn't so much). Also there is a pint/xarray integration work going on Creating a pint-xarray package/module hgrecco/pint#849

@djhoese
Copy link
Member Author

djhoese commented Oct 4, 2019

Ok so we can document what the units are for now. In the future when things are a little more stable with pint. I'm fine with that, I would like to avoid extra dependencies if we can for now.

We could even go further in the future and allow ECEF coordinates which is what I was converting when I got confused by this. Eh maybe that's a bad idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants