Repository navigation
Add multi-scale TS image computation - #234
Conversation
There was a problem hiding this comment.
Add one-line description what this function does.
|
I've left some stylistic comments inline. You need to fix the tests: Once that passes and these little things are fixed it's up to you when you want to merge. It would be nice if you moved the script to |
There was a problem hiding this comment.
Why did you change this?
I think the old version is the correct one that creates a HTML link and the new one doesn't?
(see https://gammapy.readthedocs.org/en/latest/development/index.html#where-should-i-import-from)
There was a problem hiding this comment.
I had merge conflicts and just copied the files, I guess that's where this "backward" changes come from. Thanks for the comments so far! I will address them soon and also add an example to gammapy-extra.
There was a problem hiding this comment.
If this is brand-new code, is it not dangerous to ignore astropy depreciation warnings? If you use a feature that's deprecated already, there is a good chance it will be removed in one of the next astropy versions.
There was a problem hiding this comment.
Agreed ... since we only support Astropy 1.0 (master at the moment) in Gammapy it should be possible to write code that doesn't raise any deprecation warnings.
@adonath If you get deprecation warnings from unrelated parts of Gammapy you don't have to fix them in this PR, but please don't silence them either.
There was a problem hiding this comment.
Yes, there were unrelated deprecation warnings and I included this local fix to have a clean output of the command line tool, when I was testing. Just forgot to remove it...Thanks!
1a0298a to
2bab6fb
Compare
There was a problem hiding this comment.
Change to:
ValueError('Unknown morphology: {}'.format(morphology))
|
I've left some cosmetic inline comments ... this looks good to me know, but tests should be added and if possible the peak finder from Feel free to merge this yourself next week! |
I now there is still a bit of work to be done here (put the script in a main function etc., change to back
astropy.nddata.utils.extract_array, docs missing...), however I think its ready for a first review.