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

Bkg from off data #5

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Bkg from off data #5

wants to merge 3 commits into from

Conversation

mstrzys
Copy link
Collaborator

@mstrzys mstrzys commented Apr 25, 2023

No description provided.

Marcel Strzys added 3 commits April 13, 2023 03:25
- implementation largely follows the implementation of the Exclusion
  region method, except it requires off data
- implementation of the corresponding prozessing class not fully
  finished.
TODO:
  - Testing the functionality of the class with data files.
@@ -908,3 +909,40 @@ def bkg_map_maker(self, maker):
if not isinstance(maker, ExclusionMap):
raise TypeError(f"Maker must be of type {ExclusionMap}")
BkgMakerBase.bkg_map_maker.fset(maker)
Copy link
Collaborator

@IevgenVovk IevgenVovk Apr 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be simplified to BkgMakerBase.bkg_map_maker = maker

def __init__(self, off_runs, *args, **kwargs):
super().__init__(*args, **kwargs)
self.off_runs = off_runs
self.excl_region = [Regions.parse(reg,format='ds9') for reg in
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest removing the line break after in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can introduce the line break earlier, but lines exceeding 100 characters are in disagreement with the python coding guidelines. Personally, I do not find this 100 character limit too important, so let me know what you think.

self.off_files]
)
)
self.excl_region = [Regions.parse(reg,format='ds9') for reg in
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest removing the line break after in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, I can break the line at a different place. Too long line disagrees with coding guidelines. No strong personally opinion on it.

return RectangularCameraImage(counts, self.xedges, self.yedges, self.energy_edges, exposure=exposure)

class OffDataMap:
"""_summary_
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and below: suggest filling these placeholders before merging,

RectangularCameraImage
_description_
"""
neighbours = find_run_neighbours(target_run, self.off_runs, self.pointing_delta)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The non-optional time_delta argument is missing. The function signature is

def find_run_neighbours(target_run, run_list, time_delta, pointing_delta)

I suppose some large value e.g. time_delta = numpy.inf * u.hr can be provided here, if I correctly understand the logic of the proposed OffDataMap class.

@@ -123,27 +123,48 @@ def get_runwise_bkg(self, target_run)->RectangularCameraImage:
return RectangularCameraImage(counts, self.xedges, self.yedges, self.energy_edges, exposure=exposure)

class ExclusionMap:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inheritance from BaseMap (?) is missing.

Base automatically changed from processing_refactoring to main May 12, 2023 11:23
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