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

Add Citation for covariance calculation #176

Closed
ladoramkershner opened this issue May 27, 2022 · 6 comments
Closed

Add Citation for covariance calculation #176

ladoramkershner opened this issue May 27, 2022 · 6 comments
Assignees

Comments

@ladoramkershner
Copy link
Contributor

Could we add a reference describing the calculation of the covariance?

https://github.com/USGS-Astrogeology/plio/blob/47af64c6f217210ad3409e58d8b0b5e7efd2e6fa/plio/utils/covariance.py#L4

@jessemapel
Copy link
Contributor

What kind of reference are you looking for? The computation is standard error propagation $C_b = J_b(a) C_a J_b^T(a)$ and the jacobian is just the jacobian of the spherical to rectangular transform.

@ladoramkershner
Copy link
Contributor Author

I guess a reference isn't needed, but maybe a more specific docstring and/or in line documentation would be helpful.

I spent a while trying to see how the equation in this function related to just the covariance-variance matrix, not recognizing that the calculation on line 75 and 76 is actually the error propagation formula. Additionally this function is called compute_covariance, so that is misleading unless what is happening in 78-83 is somehow converting it back to a covariance?

@jessemapel
Copy link
Contributor

I agree the documentation around this is rather confusing. I'd propose the following:

  1. Update the docs to indicate that it is using error propagation to convert the covariance from spherical to rectangular. Particularly it should indicate that this require geocentric latitude, this is not correct for geodetic latitude.
  2. The return should be changed to either return the full 3x3 covariance matrix, or a 1x6 array. It currently returns the upper triangular portion of the symmetric covariance matrix because that is what ISIS stores in control networks, but it returns it as a 2x3 which makes no sense.

I could see changing the name to something more descriptive, but it is actually computing the covariance matrix from the ground sigmas. Maybe something like compute_rect_covariance? I think it would be better to just improve the documentation instead of trying to rename it.

@ladoramkershner
Copy link
Contributor Author

I agree with just upping the documentation, I don't think there needs to be a name change. I am also a bit worried about changing the return. Wouldn't that be considered API breaking?

@jessemapel
Copy link
Contributor

That is a fuzzy part of API breaks. I would lean towards yes, and that likely makes it not worth it unless we have other breaks we want to do.

@ladoramkershner
Copy link
Contributor Author

I am fine with just a documentation update! I will try throwing up a PR tomorrow

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

No branches or pull requests

3 participants