-
Notifications
You must be signed in to change notification settings - Fork 20
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
Fast TS map and add support for regional fitting and use scatter
intead of text
to plot the true location.
#171
Fast TS map and add support for regional fitting and use scatter
intead of text
to plot the true location.
#171
Conversation
…to work with the ts fitting of a disc region.
…tter is more precise and has not offset to the input location.
Thanks @Yong2Sheng . Can you please separate this into two PRs? The plotting change is simple and I'd like to include it in DC2, but I rather not merge the other one right before the release. |
Sure! I will make a new branch that has the changes in the plot method only! |
Thanks @Yong2Sheng ! |
Hi @israelmcmc, I made another PR #172 that only contains the updates to the plotting method. I will modify this one later so it will only contain the updates to the regional fitting part. |
I changed the target branch to "develop". I want to avoid adding new features to the version we're releasing today. I'll explain this further tomorrow during the cosipy meeting. |
Codecov ReportAttention: Patch coverage is
|
Hi @Yong2Sheng , I tried your pr and everything went well for me. That's indeed fast when you can get lot of CPUs. However, why do you use a so fine binning in time (0.5s) for In addition you only used the albedo photon background component. I tried with |
Hi @GallegoSav, Thank you for your comments! it’s a good idea to use larger time binning to save the file size. I will make a new version of the data and upload to Wasabi after tests. (Total background and larger time binning). |
@Yong2Sheng: @GallegoSav is a good suggestion, but I think it can be done in a separate PR, if you want. This one is working properly, it's only a matter of the user inputs. The only thing preventing this PR from being merged is the unit test. 50% of the new lines need to be covered (it says 100%, but I have a tolerance of 50% for now). Please let me know if you need help with the unit tests. |
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. |
@israelmcmc I think I would need help for unit test because I never done that |
Hi @israelmcmc, Thank you for the comments. I will work on the unit test. I will let you know if I need help on this. |
@GallegoSav I was asking Yong to add the tests for this PR, but you'll need to do it as well at some point hehe. Here are some instructions: https://github.com/cositools/cosipy/blob/c8eb53631f686621d93a8b56395ce66ae3d71825/docs/dev/unit_tests.rst |
Hi @israelmcmc @GallegoSav , the unit test of
Could you please take a look? Thank you! |
scatter
intead of text
to plot the true location.scatter
intead of text
to plot the true location.
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 @Yong2Sheng! Everything looks good, I'm merging it.
These changes are made for introducing the TS map method into my localization pipeline and optimizing the plotting method.
Update 1
In order to accelerate the ts map fitting in localization simulation, I modified
fast_ts_fit.py
to fit a specific disc region instead of the full map.Update 2
hp.projtext
will place an offset between the text and the coordinate you give, which caused a noticeable systematic offset between the localized 90% containment region and the true location.Now it uses
hp.projscatter
, and the markers are placed at the exact location. If you run ts map tutorial notebook, you will find that the containment region is more consistent with the true location of the source than before.