search

cositools / cosipy

The COSI high-level data analysis tools
Apache License 2.0
3 stars 16 forks source link

Bug fixes from previous version of orientation module #79

Closed saurabhmittal23 closed 9 months ago

saurabhmittal23 commented 9 months ago
hiyoneda commented 9 months ago

I confirmed that the bugs were solved.

I have a comment on get_l_b(self, pointing, which). Here, the order of latitude and longitude in the array are judged by analyzing it, e.g., checking the range of the 1st and 2nd columns. However, it may cause errors in some cases, for instance, when one uses an orientation file in a very short time interval ( both (l,b) can be within -90 to +90 degrees).

Can you specify the order of (l, b) as an input parameter? Then, you can check whether the range of parameters is consistent with the given order of (l, b).

Yong2Sheng commented 9 months ago

I confirmed that the bugs were solved.

I have a comment on get_l_b(self, pointing, which). Here, the order of latitude and longitude in the array are judged by analyzing it, e.g., checking the range of the 1st and 2nd columns. However, it may cause errors in some cases, for instance, when one uses an orientation file in a very short time interval ( both (l,b) can be within -90 to +90 degrees).

Can you specify the order of (l, b) as an input parameter? Then, you can check whether the range of parameters is consistent with the given order of (l, b).

Hi Hiroki! Thank you for checking it out!

The default order is (l, b), if both columns meet the criteria, it will read the columns with the order of (l, b).

We can add that parameter assuming the users have the knowledge of the order of input. Then get_l_b function can double check that and correct it if the order doesn't match the order input from the user, and print out an warning.

What do you think?

hiyoneda commented 9 months ago

(sorry for my delayed response).

Yes, I like your idea! I think that it is good that we specify the order of (l, b) when we know it in advance, which would avoid unexpected errors.

Yong2Sheng commented 9 months ago

I've changed the inputs of SpacecraftFile from raw data to objects. Please check docs/tutorials/Point_source_resonse.ipynb for the usage. Thanks!

hiyoneda commented 9 months ago

Can you modify "parse_from_file"? Now, the variables when creating the instance of SpacecraftFile are the set of objects of astropy.time.Time, SkyCoord. However, in "parse_from_file", numpy arrays are used, which causes an error.

israelmcmc commented 9 months ago

Thank @saurabhmittal23 , @Yong2Sheng and @hirokiyoneda . Looks good to me, I'll merge