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

Fix sherpa morphology fitting script #337

Merged
merged 2 commits into from Aug 27, 2015

Conversation

Projects
None yet
4 participants
@adonath
Member

adonath commented Aug 25, 2015

This fixes the Sherpa morphology fitting script and adds a test, that runs the script on the poisson_stats_image test dataset.

@adonath

This comment has been minimized.

Member

adonath commented Aug 25, 2015

@cdeil This currently fails on Travis-Ci.

@cdeil cdeil added the feature label Aug 25, 2015

@cdeil cdeil added this to the 0.4 milestone Aug 25, 2015

@cdeil cdeil self-assigned this Aug 25, 2015

from ..sherpa_like import sherpa_image_like
from numpy.testing.utils import assert_allclose

This comment has been minimized.

@cdeil

@adonath adonath force-pushed the adonath:fix_morphology_fit_script branch from 5273638 to ec5ea04 Aug 25, 2015

@adonath adonath force-pushed the adonath:fix_morphology_fit_script branch from ec5ea04 to 06cdb47 Aug 25, 2015

@adonath

This comment has been minimized.

Member

adonath commented Aug 25, 2015

@cdeil Thanks, I've fixed the test. It still fails with an error, that I can't reproduce locally. Maybe it's related to the sherpa version (I use 'latest')?

@cdeil

This comment has been minimized.

Member

cdeil commented Aug 26, 2015

@olaurino or @DougBurke – We're starting to use Sherpa in Gammapy, and on travis-ci we're getting this AttributeError for sherpa.astro.io that @adonath doesn't see locally: https://travis-ci.org/gammapy/gammapy/jobs/77185047#L1419

As shown in https://github.com/gammapy/gammapy/blob/master/.travis.yml#L116 we're using conda install -c https://conda.binstar.org/cxc sherpa and for @adonath the test works locally, also using conda.

Any ideas what the issue is or how to fix it?

@cdeil cdeil changed the title from Fixed morphology fitting script and added test to Fix sherpa morphology fitting script Aug 26, 2015

# Save model image
sherpa.astro.ui.set_par('background.ampl', 0)
sherpa.astro.ui.notice2d()
logging.info('Writing model.fits')

This comment has been minimized.

@cdeil

cdeil Aug 26, 2015

Member

Please follow this guideline to generate log messages.

@cdeil cdeil assigned adonath and unassigned cdeil Aug 26, 2015

@cdeil

This comment has been minimized.

Member

cdeil commented Aug 26, 2015

I left one minor inline comment and the travis-ci Sherpa AttributeError has to be fixed (I don't know how).
Otherwise, this looks good to me and is ready to merge.

@DougBurke

This comment has been minimized.

DougBurke commented Aug 26, 2015

It looks like sherpa.astro.ui.utils doesn't directly import sherpa.astro.io, so it's not clear when it gets into scope. As to why this is seen on Travis and not locally is a tad strange. I don't suppose you can do something really stupid like add a import sherpa.astro.io statement to the test and see if that "fixes" it (as we don't have a more-up-to-date version of sherpa on conda yet I'm looking for simple-ish fixes to get you going while we understand the issue more ;-)?

@olaurino

This comment has been minimized.

olaurino commented Aug 26, 2015

@cdeil The issue itself is odd, especially if you don't experience it locally. I'll try and have a look asap.

In the meantime, while it might not fix your issue, you may want to switch to https://conda.binstar.org/sherpa as this is the official channel with the latest release binaries.

If you need a development cut with the latest master code I can probably arrange that, and eventually my plan is to have such development cuts to be automatically generated.

@olaurino

This comment has been minimized.

olaurino commented Aug 26, 2015

The problem seems to be that the sherpa version you are trying to use (and, for that matter, the latest official release), uses pyfits for I/O. We did add support for astropy (and fall back to pyfits), and that will work if you use the master branch:

see https://travis-ci.org/olaurino/gammapy/jobs/77343964

There are several options to fix this. I am not sure which one is preferable for you: you could install sherpa from sources, but that's probably overkill, especially since you are using the legacy Travis infrastructure so you cannot cache the build. You could install pyfits via conda to let sherpa have its I/O backend. We do ship a pyfits conda binary in the same channel as sherpa.

I could indeed provide you with a development conda package for Linux64. While I cannot commit to a date, I could try to do that by the end of the week.

I can't double check I am not missing anything obvious, but I believe the following line in your .travis.yml works "by accident":

if $OPTIONAL_DEPS && [[ $SETUP_CMD != egg_info ]] && [[ $TRAVIS_PYTHON_VERSION == 2.7 ]] && [[ $NUMPY_VERSION == 1.8 ]];
then $CONDA_INSTALL -c https://conda.binstar.org/cxc sherpa; fi

I could not find the OPTIONAL_DEPS env variable in the script... it should probably be INSTALL_OPTIONAL. Even in that case, if you set the variable to "false" the condition if $INSTALL_OPTIONAL would evaluate, I believe, to True. Same if the variable is undefined. Since this condition always evaluates to True (again, unless I am missing something), the other conditions actually determine whether sherpa gets installed, i.e. when Python 2.7 and numpy 1.8 are used.

By the way, Sherpa should support numpy 1.9, if you can trick conda into it, e.g.: conda install -y sherpa && conda install -y numpy=1.9. We will update the conda package's metadata to reflect this for the next release.

@cdeil

This comment has been minimized.

Member

cdeil commented Aug 26, 2015

My preferred solution here would be that @adonath just skips the test for now and we merge this.

Then when @olaurino can produce a development conda package for Sherpa in the coming weeks, we switch the Gammapy travis-ci to use that and activate the test.

@olaurino

This comment has been minimized.

olaurino commented Aug 26, 2015

I uploaded a sherpa conda package built from the latest master branch (commit sherpa/sherpa@2d013d1) to the following channel:
https://conda.anaconda.org/cxc/channel/dev

Please let me know if you have any issues.

@adonath

This comment has been minimized.

Member

adonath commented Aug 27, 2015

@olaurino Thanks for looking into this!
@cdeil I've still marked the test to be skipped, as I won't have time to work on this during the coming days and next week.

@cdeil

This comment has been minimized.

Member

cdeil commented Aug 27, 2015

I'll change the travis-ci build to use the dev version of Sherpa and activate the test in a new PR later today.

This looks good to me and travis-ci passed ... merging now.

@adonath – Thanks for finishing this up and enjoy your vacation!

cdeil added a commit that referenced this pull request Aug 27, 2015

Merge pull request #337 from adonath/fix_morphology_fit_script
Fix sherpa morphology fitting script

@cdeil cdeil merged commit e36bf76 into gammapy:master Aug 27, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cdeil

This comment has been minimized.

Member

cdeil commented Aug 30, 2015

FYI: In #340 I've updated the Gammapy travis-ci setup.
Before it was a modified version of the Astropy afiiliated package-template.
Now it is a modified version of the Astropy core repo travis-ci setup.

The test implemented in this PR using Sherpa is working.

@adonath adonath deleted the adonath:fix_morphology_fit_script branch Nov 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment