-
Notifications
You must be signed in to change notification settings - Fork 19
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
Continuum BG Estimation #235
Conversation
Codecov ReportAttention: Patch coverage is
|
|
||
return | ||
|
||
def laod_full_data(self, data_file, data_yaml): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
load (typo?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, thanks! Fixed now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reviewed the code. I added some suggestions about the code.
|
||
class ContinuumEstimation: | ||
|
||
def calc_psr(self, ori_file, detector_response, coord, output_file, nside=16): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future (not in this PR), we can move this part outside this class because the point source calculation is also performed in several different classes, e.g., point source injector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that makes sense.
""" | ||
|
||
# Orientatin file: | ||
sc_orientation = SpacecraftFile.parse_from_file(ori_file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use sc_orientation
directly instead of the path to an orientation file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, done.
self.psr = response.get_point_source_response(coord = coord, scatt_map = scatt_map) | ||
|
||
# Save: | ||
self.psr.write(output_file + ".h5") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you return psr
here? I think it would be better than saving a PSR file outside this function: users receive psr
from this function and save it by themselves if they want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, done.
Full path to precomputed response file (.h5 file). | ||
""" | ||
|
||
logger.info("...loading the pre-computed image response ...") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> "...loading the pre-computed point source response ..." ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
search_right = True | ||
while search_right == True: | ||
|
||
if this_index+j >= len(self.psr.axes['PsiChi'].centers)-1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this_index+j >= self.psr.axes['PsiChi'].nbins-1:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More elegant, thanks! Changed.
plt.show() | ||
plt.close() | ||
|
||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to return arm_mask
and sorted_indices
? Assigning them as member parameters might be confusing because it will be hard to know from which sliced data self.arm_mask
and sorted_indices
are generated. My suggestion is that arm_mask
and sorted_indices
are explicitly generated for each loop in continuum_bg_estimation
and they are used as input parameters of simple_inpainting
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made your suggested changes.
|
||
return | ||
|
||
def simple_inpainting(self, m_data): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the above comment, can you add arm_mask
and sorted_indices
as input parameters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
self.full_data = BinnedData(data_yaml) | ||
self.full_data.load_binned_data_from_hdf5(data_file) | ||
self.estimated_bg = self.full_data.binned_data.project('Em', 'Phi', 'PsiChi') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move the line 92 inside continuum_bg_estimation
because the background model is not yet generated at this point? And, maybe, estimated_bg
can be just an output of the function continuum_bg_estimation
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
self.estimated_bg.write(output_file,overwrite=True) | ||
logger.info("Finished!") | ||
|
||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you return estimated_bg
here? It would be better that estimated_bg
is saved by users outside this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Thanks again for your review, @hiyoneda! It was very helpful. I implemented all your comments, and I also added unit tests. I think it can be merged if you think it's ok. |
@ckarwin Thanks. It looks good. I have two more comments on the notebook. After they are reflected, I think that it can be merged.
|
@hiyoneda Ok, it's been updated.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! It can be merged.
This is an initial method for estimating the background for continuum sources. It is still highly preliminary, and so I am opening this as a PR draft, in order to share the example notebook which has the initial algorithm hard-coded (available here). I plan to develop the
ContinuumEstimation
class leading up to the October meeting, in order to at least have a working version that can then be further develop. I think theContinuumEstimation
class should be part of abackground_estimation
base class, which will also contain aLineEstimation
class and aBurstEstimation
class. But we should discuss this and figure out what makes the most sense.Here are slides summarizing the initial results from the method presented in the example tutorial:
continuum_bg_estimation.pdf. Below is a summary of the next steps:
Improving algorithm:
Optimize cut for mask selection (so far only tested cumdist < 0.4)
Use actual ARM measurement for mask selection
Use more sophisticate inpainting method:
Give more freedom to fit (first need to fix unrelated convergence problem).
Vectorize code
More tests: