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 error in reading observation when it does not have tod field #262

Merged
merged 5 commits into from
Aug 1, 2023

Conversation

marcobortolami
Copy link
Contributor

This PR solves #261.

@marcobortolami
Copy link
Contributor Author

marcobortolami commented Jul 27, 2023

Do you think we should also modify test_io.py? An easy peasy way would be to just change the names tod and other_tod to tod1 and tod2, so the test is slightly more strong and tests also the case that was problematic, fixed with this PR.

@marcobortolami
Copy link
Contributor Author

I committed my idea of improvement for test_io.py:) I can revert the changes if you don't like them;)

@marcobortolami
Copy link
Contributor Author

Two questions:

  1. Why we define the two tod field types as float 64 and 32 but then we assert that they are both float 32? The test passes and I'm confused...
  2. Here we test the reading of an observation with and without mjd and gzip. However, not all the 4 combinations of the flags are present (False-False,False-True,True-False,True-True) because the True-True case is missing and the last test test_quaternions_in_hdf5 is completely degenerate with test_gzip_compression_in_obs. Maybe one of the flags was mistakenly put to False? I propose to modify the last two test names as test_gzip_compression_in_obs_mjd and test_gzip_compression_in_obs_no_mjd with obvious setting of the flags.

@ziotom78
Copy link
Member

ziotom78 commented Aug 1, 2023

I committed my idea of improvement for test_io.py:) I can revert the changes if you don't like them;)

Please keep it! I like it!

Why we define the two tod field types as float 64 and 32 but then we assert that they are both float 32? The test passes and I'm confused...

Because the function read_list_of_observations automatically converts every TOD it reads in the dtype specified by the parameter tod_dtype, and the default is np.float32.

Here we test the reading of an observation with and without mjd and gzip. However, not all the 4 combinations of the flags are present (False-False,False-True,True-False,True-True) because the True-True case is missing and the last test test_quaternions_in_hdf5 is completely degenerate with test_gzip_compression_in_obs. Maybe one of the flags was mistakenly put to False? I propose to modify the last two test names as test_gzip_compression_in_obs_mjd and test_gzip_compression_in_obs_no_mjd with obvious setting of the flags.

No, the problem here is that test_quaternions_in_hdf5 was a placeholder for a more complex test I have never had the chance to complete. (There are no quaternions in HDF5 so far…).

I wouldn't bother add the True-True case, as the code used to compress the file using GZip does not depend on the fact that MJD dates be used or not. The best thing is to just remove test_quaternions_in_hdf5 altogether.

Thanks for having spotted these issues! From my side, this PR can be merged.

@marcobortolami
Copy link
Contributor Author

Please keep it! I like it!

Ok:)

Because the function read_list_of_observations automatically converts every TOD it reads in the dtype specified by the parameter tod_dtype, and the default is np.float32.

Oh, right, now I noticed the conversion!

I wouldn't bother add the True-True case

Ok!

The best thing is to just remove test_quaternions_in_hdf5 altogether.

Done!

@marcobortolami marcobortolami merged commit 8eae784 into master Aug 1, 2023
@marcobortolami marcobortolami deleted the fix_read_one_observation branch August 1, 2023 13:47
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