search

litebird / litebird_sim

Simulation tools for LiteBIRD
GNU General Public License v3.0
18 stars 13 forks source link

Fix error in reading observation when it does not have tod field #262

Closed marcobortolami closed 1 year ago

marcobortolami commented 1 year ago

This PR solves #261.

marcobortolami commented 1 year ago

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 commented 1 year ago

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

marcobortolami commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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!