Add multi-resolution TS map fitting #248
Add multi-resolution TS map fitting #248Yong2Sheng wants to merge 7 commits intocositools:developfrom
Conversation
…mpatiable with the MOC map fitting.
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (31.70%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.
... and 9 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Hello Yong, I looked at this tutorial, including the MOC-TS map. It works fine for me, I have only very minor suggestions from the point of view of the user. Apart from those, I think I can complete the review. i) MOC-TS map: maybe add a markdown reminding the user of the meaning of the parameters max_moc_order and top_number. I think also it would be useful to print on the screen the delta_likelihood between the top pixels (that will continue to be fitted and higher resolution) and the others. So that the user can have a feeling of how significant is the source the the procedure is finding. ii) Example 3, when the data are downloaded and binned. Maybe you can add another if statement checking for the existence of the .gz version of the file. The .gz file can also be unzipped on the fly using: import subprocess and subprocess.run(["gunzip", crab_unbinned_zipped_path]). The "shutils" command that you use before works only with .zip files, not with .gz. I would also a note to remind the user that the .ymal files have to be edited by hand (updating the paths) before proceeding. iii) There are a few typos, for instance, in example 2, 1st cell: excpt>>execEpt iv) Maybe you could consider splitting the tutorial into two notebooks e.g. one with the Crab and one with the GRB, or one with the Fast TS map and one with the MOC TS map. |
|
Hi @ldigesu, Thank you for the review. I updated the PR based on your comments.
I added comments to the cell to explain
I printed the top ts values.
Run: Warning output: |
|
@Yong2Sheng I'm going back to this PR. I haven't finished, but here are a few things to start with:
|
|
@Yong2Sheng It seems you did delete the old nb, but somehow it came back. Maybe during the sync with develop. Anyway, nevermind, I'll handle that during the rebase and adjustment to your nb based on recent changes in #334 |
|
@Yong2Sheng I made another branch rebasing onto v0.3.x: This should have all of your commits. Please double check that it still looks good to you? If so, please branch out of this branch and open a new PR with v0.3.x as target. I also made this other (auxiliary & temporary) branch where I merged it with the current I think this works well and (the rebased version) can be merged after:
|
|
Closing and opening this to run the tests again. |
|
Hi @israelmcmc, thank you for reviewing it! I will address the comments this week. |
|
Thanks @Yong2Sheng. Btw, it seems that kicking the tests worked. Everything is passing now, except the coverage, as expected. |
|
Hi @israelmcmc, I opened a new #350, could you please take a look? |
|
Thanks @Yong2Sheng! I'm closing this in favor of #350. I'll take a look. |
|
Thank you @israelmcmc ! |
This PR adds the method to do multi-resolution (also called multi-order coverage, MOC) TS map fitting.
Changes to
FastTSMapFastTSMapis the parent class ofMOCTSMapI need to make several changes to
FastTSMapto make it work withMOCTSMap:pixel_idxparameter forfast_ts_fit.FastTSMap.zip_compto generate the inputs formultiprocessing.itertools.productbefore. However, it will cause duplicated fittings inMOCTSMap.zip_compto replace it.New module
MOCTSMap.Changes to the TS map tutorial
Please let me know if you have any questions and suggestions. Thank you!