-
Notifications
You must be signed in to change notification settings - Fork 13
Delta residual fix #99
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
Conversation
|
I don't think it's quite right, because it can furnish negative numbers: I might put in the circular distance I sent you. It's great that you added the test. |
|
Is that an issue given that the standard way of calculating residuals (i.e., model - data) can also furnish negative numbers? Won't the result always be squared anyway? |
|
In this case I don't think there are modes of analysis where the sign matters. Also, what about psi? |
|
Psi is the ratio of s and p polarised light, so isn't an angle. I think the way we calculate psi residuals are ok? |
|
Ok, let's leave it there then. |
|
Also the docstring specified degrees in radians, but delta is always supplied in degrees (in refellips). I don't think it matters for the logic as long as the period is also in degrees, but might cause confusion down the line. |
|
I'm happy. Lets merge? |
|
What other changes did you want to make? |
|
I've got a few file readers (filmsense and Woolam M2000) + tests. |
Delta was not being treated as an angle when residuals were being calculated. For instance, the difference between 2˚ and 358 ˚ was calculated at 356˚, rather than 4˚. This typically doesn't cause an issue when datasets do not contain Delta close to 0/360 (so avoided detection) but can cause serious issues.
The current pull request fixes this calculation using:
(delta_model - delta_data + 180) % 360 – 180
Which is an efficent way of calculating the circular distance between two angles.