Extended source response #223

hiyoneda · 2024-07-18T14:09:38Z

I created the first version of the extended source response related to #216 .

A function (get_integrated_spectral_model) was moved from threeml/custom_functions.py to response/functions.py.
- Basically, I copied these codes from PointSourceResponse.py. So, this function can also be used in PointSourceResponse.py @israelmcmc
A new function, get_integrated_extended_model was added in threeml/custom_functions.py, which calculates integrated flux over the sky from a given astromodel.ExtendedSource
Added ExtendedSourceResponse. Currently, the functionalities are limited. I assume that we will add more functions in the future
- The overflow/underflow bins are not loaded when an hdf5 file has them. It saves the memory usage significantly if the energy axis is one bin.
threeml/COSILike was updated to use ExtendedSourceResponse .

Now, the updates are minimal. Any feedback is very welcome.

@saurabhmittal23 Please use this PR and test your extended model fitting. I am happy if you could check the 511 keV notebook first and see if the results are the same as before.

…om threeml

israelmcmc · 2024-07-22T13:59:53Z

I'm closing and reopening this PR to kick codecov. There was a recent change in GitHub that broke it, but I think it's fixed now.

codecov · 2024-07-22T14:03:42Z

Codecov Report

Attention: Patch coverage is 83.14607% with 15 lines in your changes missing coverage. Please review.

Project coverage is 74.02%. Comparing base (d02846a) to head (5bdaf39).
Report is 36 commits behind head on develop.

Files with missing lines	Patch %	Lines
cosipy/response/functions.py	76.19%	10 Missing ⚠️
cosipy/response/ExtendedSourceResponse.py	90.90%	3 Missing ⚠️
cosipy/threeml/COSILike.py	33.33%	2 Missing ⚠️

Files with missing lines	Coverage Δ
cosipy/image_deconvolution/allskyimage.py	`100.00% <100.00%> (ø)`
cosipy/response/PointSourceResponse.py	`100.00% <100.00%> (+40.54%)`	⬆️
cosipy/response/__init__.py	`100.00% <100.00%> (ø)`
cosipy/source_injector/source_injector.py	`100.00% <ø> (ø)`
cosipy/threeml/custom_functions.py	`33.66% <ø> (-4.92%)`	⬇️
cosipy/threeml/COSILike.py	`20.88% <33.33%> (+0.76%)`	⬆️
cosipy/response/ExtendedSourceResponse.py	`90.90% <90.90%> (ø)`
cosipy/response/functions.py	`76.19% <76.19%> (ø)`

israelmcmc · 2024-07-23T13:34:44Z

Thanks for the changes @hiyoneda. I haven't gone through all of the code, but I noticed that in ExtendedSourceModel you are tracking _contents, _axes, and _units manually. You can instead inherit from Histogram, as PointSourceModel does, and that way you'll get all features, which is also less error-prone.

hiyoneda · 2024-07-23T18:42:34Z

I needed to do it on purpose. If histpy.Histogrom is used, the overflow/underflow bins are automatically created. For the gamma-ray line case (when the number of energy bins is one), the loaded matrix size gets 9x larger than the necessary size. (actually, the extended model fitting notebook could not be performed on Saurabh's environment due to this issue). So the response components are manually loaded, and the overflow/underflow bins are ignored in this update.

hiyoneda · 2024-07-23T19:23:50Z

Another future concern is when the response is very large (~O(100-1000) GB), for example, nside = 64.
In this case, we cannot open the response at once even if the underflow/overflow bins are not loaded, and we would need some functionalities for lazy reading (or the response components can be generated each time they are referred).

If these features can be implemented in Histogram, I agree that we should use it as a base class. But if not, I am worrying that it rather reduces flexibility in the future.

israelmcmc · 2024-07-23T20:25:14Z

I see. Yeah, the underflow/overflow bins are an issue in that case, and there's no easy way to change that in histpy. It's certainly possible, but since the assumption of those bins permeates the code, it will take a while to implement.

Partial reading was part of the reason for using HDF5, and FullDetectorResponse only loads one direction at a time. Indeed it might not be enough. On a tangent note, if we move to an unbinned analysis, we can load from the HDF5 only the nearest bins for each event, one event at a time ¯_(ツ)_/¯

hiyoneda · 2024-07-23T21:18:52Z

Personally, I want histpy to keep the underflow/overflow bins, because I like the concept of histpy very much, which is useful for manipulating histograms in data analysis. It is also useful that histograms have the underflow/overflow bins in many cases. I think that the fundamental issue here would be that the response is a matrix or tensor rather than a histogram.

Partial reading was part of the reason for using HDF5, and FullDetectorResponse only loads one direction at a time.

Actually, I was wondering if we could keep the instance of h5py instead of its contents in a future update. FullDetectorResponse will be a good reference for how to update ExtendedSourceResponse in the future.

On a tangent note, if we move to an unbinned analysis, we can load from the HDF5 only the nearest bins for each event, one event at a time

It sounds interesting. Considering that it seems always useful to keep the HDF5 in the response classes, should any response class have three attributes, the HDF5 data, axes, unit?

…response/function.py

israelmcmc · 2024-08-05T23:13:36Z

@hiyoneda I didn't want to give away all the slicing, projecting, plotting, etc. capabilities that have shown useful for debugging and concise coding, so I spent some time adding the capability to histpy to disable under/overflow tracking (axis by axis). I haven't merged it yet, but you can checked it out here: https://gitlab.com/burstcube/histpy/-/merge_requests/1 . Please let me know what you think!

hiyoneda · 2024-08-06T12:13:18Z

@israelmcmc Thanks for making the change on histpy. I tried the new code from the no_overflow branch, and I like it so much. I made a histogram with similar dimensions of gamma-ray line response and confirmed that the over/underflow bins are not used with the new option. It saves memory and storage significantly, as expected. Once it is merged, I can update the extended source response using this new functionality.

hiyoneda · 2024-10-03T15:45:24Z

I modified the class as a child class of Histogram. Now it can save the memory space.
@israelmcmc I want to ask two questions:

Has the no_overflow branch of histpy already been merged to the main?
Currently, the image axis in the response is always "NuLambda," but sometimes it is confusing because 'NuLambda' sounds like it in the local coordinates even when it is a galactic response. Can we change the name of the image axis to match the actual coordinates, e.g., lb, radec etc.?

israelmcmc · 2024-10-03T17:57:40Z

Thanks, @hiyoneda:

I haven't merged it yet, but I think I'll do it soon. I've used it for a while now with no issues. I'll try to make a new release tomorrow
I agree it would be better to label the axis based on the coordinates it represents, and leave NuLamda only for SC coordinates. Can you please open an issue about it?

israelmcmc · 2024-10-09T17:21:40Z

@hirokiyoneda I made a new histpy release (1.1.4) that includes the optional under/overflow

hiyoneda · 2024-10-11T15:44:29Z

I agree it would be better to label the axis based on the coordinates it represents, and leave NuLamda only for SC coordinates. Can you please open an issue about it?

opened #250

…ation

…removed

Yong2Sheng

I just have a few general questions. Everything looks good to me, I will run TS map and spectral fitting notebook to see if everything goes through.

Yong2Sheng · 2024-10-23T19:13:13Z

cosipy/response/ExtendedSourceResponse.py

+                        unit = unit,
+                        track_overflow = track_overflow)
+
+        del hist


After del hist, the data referenced by hist will still remain in the RAM (or it will cleaned when the python does the reference count)
Using gc.collect() will remove the data right away. Since the response is quiet large, so might help to remove from RAM right away.

Why do you reconstruct the Histogram instead of using the opened one?

I've changed the code corresponding to the first comment.

Why do you reconstruct the Histogram instead of using the opened one?

This is because the opened one is an instance of histpy.Histogram, not ExtendedSourceResponse. So, I needed to instantiate ExtendedSourceResponse at a new line. @israelmcmc Is there any nice way to directly open a hdf5 file as an instance of ExtendedSourceResponse?

@hiyoneda I think that you can do cls.open(filename, name) instead of super().open(filename, name). The output would be an ExtendedSourceResponse object.

Nevermind. In principle this should work, but there's an issue with histpy that prevents it at the moment. I think this can be merged as is, and we can come back to it one I fix the issue in histpy

I've changed the code corresponding to the first comment.

Thanks @hirokiyoneda !

Nevermind. In principle this should work, but there's an issue with histpy that prevents it at the moment. I think this can be merged as is, and we can come back to it one I fix the issue in histpy

Sounds good!

This is the corresponding issue, for the record https://gitlab.com/burstcube/histpy/-/issues/23

…dSourceResponse

Yong2Sheng · 2024-10-25T18:42:55Z

I ran through the spectral and TS map fitting notebooks without issues. They return the same fitted values. I will merge.

Yong2Sheng

Merge.

Yong2Sheng

Merge.

hiyoneda added 2 commits July 18, 2024 15:59

Moved functions to calculate integrated flux to response directory fr…

0e9cedd

…om threeml

Added ExtendedSourceResponse and modified COSILike

4fa803a

hiyoneda requested review from saurabhmittal23, israelmcmc, ckarwin and fieldrog July 18, 2024 14:09

hiyoneda added response spatial modeling threeml_plugin labels Jul 18, 2024

Minor modifications on response/functions.py

cd0b311

israelmcmc closed this Jul 22, 2024

israelmcmc reopened this Jul 22, 2024

Modified PointSourceResponse to use get_integrated_spectral_model in …

b18a484

…response/function.py

Changed ExtendedSourceResponse as a child class of Histogram

624d9e0

hiyoneda added 3 commits October 3, 2024 17:50

Merge branch 'develop' into extended_source_response

eae5a5f

bug fixed

9d11eae

bux fixed

a57736a

hiyoneda added 4 commits October 22, 2024 11:36

Added the axes consistency check in ExtendedSourceResponse.get_expect…

c99b1e1

…ation

Added unittest for ExtendedSourceResponse

f48f913

Merge branch 'develop' into extended_source_response

1158ff2

Modified SourceInjector corresponding to the change that Ei axis was …

a55e5b7

…removed

hiyoneda requested a review from Yong2Sheng October 22, 2024 23:47

Yong2Sheng reviewed Oct 24, 2024

View reviewed changes

hiyoneda added 5 commits October 24, 2024 16:41

Modified PointSourceResponse to use np.tensordot

10e29b6

Added gc.collect() in ExtendedSourceResponse

82a15d8

Always use the input response histogram as a dense matrix for Extende…

45e6939

…dSourceResponse

Modify PointSourceResponse to use np.tensordot

b807978

Modified unittest for SourceInjector and ExtendedSourceResponse

5bdaf39

Yong2Sheng approved these changes Oct 25, 2024

View reviewed changes

israelmcmc removed request for saurabhmittal23, israelmcmc, ckarwin and fieldrog October 25, 2024 18:59

israelmcmc approved these changes Oct 25, 2024

View reviewed changes

hiyoneda merged commit 9eb70c1 into cositools:develop Oct 25, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extended source response #223

Extended source response #223

hiyoneda commented Jul 18, 2024 •

edited

Loading

israelmcmc commented Jul 22, 2024

codecov bot commented Jul 22, 2024 •

edited

Loading

israelmcmc commented Jul 23, 2024

hiyoneda commented Jul 23, 2024 •

edited

Loading

hiyoneda commented Jul 23, 2024

israelmcmc commented Jul 23, 2024

hiyoneda commented Jul 23, 2024

israelmcmc commented Aug 5, 2024

hiyoneda commented Aug 6, 2024

hiyoneda commented Oct 3, 2024

israelmcmc commented Oct 3, 2024

israelmcmc commented Oct 9, 2024

hiyoneda commented Oct 11, 2024

Yong2Sheng left a comment

Yong2Sheng Oct 23, 2024

Yong2Sheng Oct 23, 2024

hiyoneda Oct 24, 2024

hiyoneda Oct 24, 2024

israelmcmc Oct 25, 2024

israelmcmc Oct 25, 2024

Yong2Sheng Oct 25, 2024

Yong2Sheng Oct 25, 2024

israelmcmc Oct 25, 2024

Yong2Sheng commented Oct 25, 2024

Yong2Sheng left a comment

Yong2Sheng left a comment

Extended source response #223

Extended source response #223

Conversation

hiyoneda commented Jul 18, 2024 • edited Loading

israelmcmc commented Jul 22, 2024

codecov bot commented Jul 22, 2024 • edited Loading

Codecov Report

israelmcmc commented Jul 23, 2024

hiyoneda commented Jul 23, 2024 • edited Loading

hiyoneda commented Jul 23, 2024

israelmcmc commented Jul 23, 2024

hiyoneda commented Jul 23, 2024

israelmcmc commented Aug 5, 2024

hiyoneda commented Aug 6, 2024

hiyoneda commented Oct 3, 2024

israelmcmc commented Oct 3, 2024

israelmcmc commented Oct 9, 2024

hiyoneda commented Oct 11, 2024

Yong2Sheng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yong2Sheng commented Oct 25, 2024

Yong2Sheng left a comment

Choose a reason for hiding this comment

Yong2Sheng left a comment

Choose a reason for hiding this comment

hiyoneda commented Jul 18, 2024 •

edited

Loading

codecov bot commented Jul 22, 2024 •

edited

Loading

hiyoneda commented Jul 23, 2024 •

edited

Loading