Fix astype in coordinates matching when sampling data frames

[acocac]

The PR fixes the sampling strategy when locations are np.ndarray. It also amends the docs of the sample_df function.

Fixes #125

[tom-andersson]

This looks like a great catch! Two comments:

1. Please could you add a unit test in test_task_loader that tries to sample with a different dtype, run it localy with pytest, and confirm that it fails with InvalidSamplingStrategyError when your fix is commented out, and passes after the fix, then add it to this PR
2. run ‘black’ for style to pass

[acocac]

@tom-andersson if I understand well, the proposed unit test should test a Dataframe to sample with different dtypes in the x1 and x2 dimensions. There's already a test checking invalid sampling strategies, so should I create another one for invalid sampled dataframes?

[tom-andersson]

@acocac I wouldn't put the unit test in test_invalid_sampling_strat, because we actually expect that there will not be any errors raised when the x1/x2 dtype is different from that of the DataFrame being sampled. So I would add a new unit test like test_different_dtype_when_sampling_offgrid_data_at_specific_numpy_locs. Here is an example where we just run the TaskLoader and add a comment that no error should be raised. And here is an example where we sample a toy DataFrame at [0, 0], so you can use sampling_strat=np.zeros((2, 1), dtype=wrong_dtype) and then try task = tl("2020-01-01", sampling_strat, sampling_strat).

That unit test should fail before your fix, and pass afterwards.

[acocac]

@tom-andersson thanks for the suggestions. I've added the unit test, however, I've found the example of sampling the toy data frame at [0,0] isn't the most useful to indicate the effect of forcing x1/x2 dtype. I think the problem relates to inaccuracies in the floats with values between 0 and 1 and their impact in np.in1d

The proposed test actually fails with/without the fix, so I'd appreciate if you can have a look at the test 🙏

[tom-andersson]

@acocac the unit test was failing because of a bug in the way the DataFrame is being sampled, which triggered an InvalidSamplingStrategy error. I'll fix this in a separate commit.

Regarding the type issue, you were not actually passing the TaskLoader a numpy array with a different dtype to the index values. I fixed that, and then found that the unit test was still failing even after casting the dtype. Thinking more about this, I think it is not safe to do casting, as it doesn't guarantee that coords of different dtypes will match. I've decided to instead raise an informative error for the user if the dtypes differ, rather than casting under the hood, and I've updated the unit test to reflect this.

Happy to merge this once tests are passing. Thanks again for finding this!

[tom-andersson]

@all-contributors please add @acocac for code, tests

[allcontributors]

@tom-andersson

I've put up a pull request to add @acocac! 🎉

[acocac]

@acocac the unit test was failing because of a bug in the way the DataFrame is being sampled, which triggered an InvalidSamplingStrategy error. I'll fix this in a separate commit.

Regarding the type issue, you were not actually passing the TaskLoader a numpy array with a different dtype to the index values. I fixed that, and then found that the unit test was still failing even after casting the dtype. Thinking more about this, I think it is not safe to do casting, as it doesn't guarantee that coords of different dtypes will match. I've decided to instead raise an informative error for the user if the dtypes differ, rather than casting under the hood, and I've updated the unit test to reflect this.

Happy to merge this once tests are passing. Thanks again for finding this!

The proposed solution sounds reasonable. I agree it could be safer to do casting from the user-side. The warning message helps!