Skip to content
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

Check that list_samplers returns a list (fix #397) #398

Merged
merged 5 commits into from Jan 9, 2018

Conversation

DougBurke
Copy link
Contributor

@DougBurke DougBurke commented Sep 10, 2017

Release notes

The list_samplers function now returns a list in Python 3 as well.

Notes

This is part of #397.

@DougBurke
Copy link
Contributor Author

Hmmm, I expected the first commit - 16cb167 - to fail the python3 tests.

@DougBurke
Copy link
Contributor Author

No idea why the first commit passed - looking at https://travis-ci.org/sherpa/sherpa/jobs/273877659#L836 we can see that it has run the new test (test_sim_unit.py) and that this is a python 3.5 installation. On master branch I find (with python 3.5)

% ipython
Python 3.5.4 |Continuum Analytics, Inc.| (default, Aug 14 2017, 13:26:58) 
Type 'copyright', 'credits' or 'license' for more information
IPython 6.1.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: from sherpa.sim import MCMC

In [2]: m = MCMC()

In [3]: m.list_samplers()
Out[3]: dict_keys(['mh', 'metropolismh'])

In [4]: f = m.list_samplers()

In [5]: isinstance(f, list)
Out[5]: False

so I don't see how the test could have passed.

This is a copy of the sherpa.sim test, but it is lsightly different in that
the set of available samplers has been increased.

Perhaps the test for the samplers should be seprated from the test that
list_samplers returns a list?
There should be no functional change here
The contents of the list_samplers return value is now separated out into
a separate test. As this routine is also available from the ui layer new
tests are added for it (essentially copies of the same test).

I have created new test files for the "ui" versions of the tests: these
may be better in the session tests (but see discussion of session tests
in sherpa#396) or the exsiting "unit" test for the ui code (e.g.
test_astro_ui_utils_unit.py)
@DougBurke
Copy link
Contributor Author

About to rebase. Original tip was b152b40

@DougBurke DougBurke force-pushed the sampler-return-list-python3.5 branch from b152b40 to 3177f0d Compare November 3, 2017 22:54
@DougBurke
Copy link
Contributor Author

I have no idea why the travis builds succeeded for the first commit (the equivalent of 71ee06b before the rebase). I'm removing the pr:hold label.

@DougBurke DougBurke removed the pr:hold label Nov 5, 2017
@DougBurke
Copy link
Contributor Author

@olaurino can we have this labelled for CIAO 4.10?

@olaurino olaurino added this to the 4.10 milestone Dec 11, 2017
@olaurino
Copy link
Member

olaurino commented Jan 8, 2018

I must have done something stupid while I was trying to debug the Travis build, because my initial guess on why Travis was green turned out to be correct but only after I thought I had proved it was not.

Anyhow. Unless you have Travis activated on your fork (which would be a good idea) the only check performed by Travis is the PR build, i.e. how well the PR plays when merged into the master branch. Past experience tells us that this is less safe than the push builds, because the push builds are executed on the individual commits, while for the PR builds Travis uses a special GitHub reference for a fake merge commit of the PR (e.g. https://travis-ci.org/sherpa/sherpa/jobs/273877659#L488), not the commit ID.

That means that if you push multiple commits in rapid succession Travis might run multiple builds on the same commit (well, I think Travis now cancels builds if they are overridden by other commits in the same branch, so that's also different, and it might be what tripped me this time). So, my educated guess is that all the Travis builds were run on merge commits that already had the fix in them.

I can't fully test that the above is actually what happened here, but what I could do was to push the first commit in this PR to my fork and check that it failed in Travis. It did, exactly as expected.

@olaurino olaurino merged commit 3177f0d into sherpa:master Jan 9, 2018
@DougBurke DougBurke deleted the sampler-return-list-python3.5 branch February 8, 2018 20:16
@olaurino olaurino changed the title Check that list_samplers returns a list Check that list_samplers returns a list (fix #397) Feb 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants