Skip to content

Conversation

@igresh
Copy link
Contributor

@igresh igresh commented Sep 26, 2025

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.

@igresh igresh requested a review from andyfaff September 26, 2025 05:54
@andyfaff
Copy link
Contributor

I don't think it's quite right, because it can furnish negative numbers:

def cd2(angle1, angle2):
    return (angle1 - angle2 + 180) % 360 - 180

cd2(358, 2) # -4

I might put in the circular distance I sent you. It's great that you added the test.

@igresh
Copy link
Contributor Author

igresh commented Sep 26, 2025

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?

@andyfaff
Copy link
Contributor

In this case I don't think there are modes of analysis where the sign matters.

Also, what about psi?

@igresh
Copy link
Contributor Author

igresh commented Sep 26, 2025

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?

@andyfaff
Copy link
Contributor

Ok, let's leave it there then.

@igresh
Copy link
Contributor Author

igresh commented Sep 26, 2025

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.

@igresh
Copy link
Contributor Author

igresh commented Sep 26, 2025

I'm happy. Lets merge?

@andyfaff andyfaff merged commit bdaa4e3 into refnx:main Sep 26, 2025
6 checks passed
@andyfaff
Copy link
Contributor

What other changes did you want to make?

@igresh
Copy link
Contributor Author

igresh commented Sep 26, 2025

I've got a few file readers (filmsense and Woolam M2000) + tests.

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

Successfully merging this pull request may close these issues.

2 participants