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

Fix rounding errors in DeepSensorModel.predict coordinates from normalise-unnormalise operations #25

Merged
merged 6 commits into from
Jul 17, 2023

Conversation

polpel
Copy link
Collaborator

@polpel polpel commented Jul 13, 2023

This adds a unit test which checks that normalising and unnormalising an xarray object with DataProcessor (as is done e.g. in model.predict) should return the exact same coordinates as the original object.

This test currently fails, as pointed out in Issue #19

@tom-andersson tom-andersson changed the title Add DataProcessor unit test for norm-unnorm coords Fix rounding errors in DeepSensorModel.predict coordinates from normalise-unnormalise operations Jul 17, 2023
@tom-andersson
Copy link
Collaborator

Hi @polpel, thanks very much for opening this. It's important to fix this to avoid the silent dropping of data in #19.

The reason for the slightly differing coordinate values is simply floating point rounding error / numerical noise. I tried moving to float64s and while this reduced the magnitude of the rounding errors, they were still present, and it looks like xarray needs the coordinates to align exactly for operations like subtraction (as mentioned in #19), which has been discussed on the xarray repo (e.g. pydata/xarray#2217, pydata/xarray#6573).

Since we can't prevent rounding errors, the problem is actually with the way the high-level DeepSensorModel.predict method normalises the X_t coords for prediction and then unnormalises this to return to the user. I've fixed this now with 74cc5b8 and moved/refactored your unit test into tests/test_model.py.

Tests are passing, so I will merge with main. This closes #19.

@tom-andersson tom-andersson reopened this Jul 17, 2023
@tom-andersson tom-andersson merged commit a0224fe into alan-turing-institute:main Jul 17, 2023
7 checks passed
@@ -258,6 +258,30 @@ def test_wrong_extra_indexes_pandas(self):
with self.assertRaises(ValueError):
dp(df_raw)

def test_norm_unnorm_preserves_coords_xr(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for future reference that this is missing an indentation level @polpel - it should be part of the test class.

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