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

shuffle indices of mock targets in place #416

Merged
merged 5 commits into from
Nov 7, 2018
Merged

shuffle indices of mock targets in place #416

merged 5 commits into from
Nov 7, 2018

Conversation

moustakas
Copy link
Member

One-line fix to #414. @forero can you please test with your specific inputs?

@moustakas
Copy link
Member Author

Some more details about the origin of the bug:

In select_mock_targets each (input) healpixel is divided into nside_chunk sized chunks (by default 128) and then the targets in each chunk are further divided into MAXITER blocks (by default 5). The code iteratively builds spectra and selects targets (up to MAXITER) but quits once the desired target density is achieved.

It turns out that the DarkSky mocks were roughly sorted by right ascension, and in the --no-spectra version of the code the requisite target density was achieved within the first couple iterations, hence the spatial gridding (which was most severe and obvious for the ELGs, which have the highest target density).

The solution was to simply shuffle in place the indices of the targets on a given chunk.

Travis failures are due to changes totally unrelated to this PR. Maybe @sbailey or @weaverba137 can take a quick peak?

@weaverba137
Copy link
Member

The errors are caused by SciPy not finding a C++ library. I'm not certain exactly why, but a solid clue is that the script that loads the Conda environment is installing numpy 1.15, but then later downgrading it to 1.13. It is possible the version of SciPy is not compatible with 1.13.

@sbailey
Copy link
Contributor

sbailey commented Nov 6, 2018

@sbailey sbailey mentioned this pull request Nov 6, 2018
@sbailey
Copy link
Contributor

sbailey commented Nov 6, 2018

Also see the long thread at ContinuumIO/anaconda-issues#5191 . That is closed but has the ominous comment "I suspect one of your PIP installed dependencies is being imported before scipy and is linking to your system libstdc++ and that prevents our libstdc++ from being used." We do use pip for healpy and fitsio, and healpy was updated after we switched to using pip, though we have had successful pull requests with Travis tests since then...

@weaverba137
Copy link
Member

I agree with the Anaconda engineer(s) on the thread that Stephen mentioned: do not muck about with the system C++ libraries; Anaconda includes its own, that's where we need to look. And I also agree that pip installations and the numpy downgrade that I already mentioned are where to look.

Take a look at the conda list command here: https://travis-ci.org/desihub/desitarget/jobs/450193960#L1316

Some obvious problems:

  1. Some packages are duplicated: e.g. requests is installed by both conda and pip.
  2. numpy-base 1.15.2 is installed with numpy 1.13.1
  3. Why does desitarget need photutils? I don't recall that ever being a DESI dependency.

And then if you look at https://travis-ci.org/desihub/desitarget/jobs/450193960#L1420:

 if [[ $SETUP_CMD == test* ]]; then python -c 'import numpy; import scipy; print(numpy.__version__); print(numpy.__path__); print(scipy.__version__); print(scipy.__path__)'; fi
1.15.2
['/home/travis/miniconda/envs/test/lib/python3.5/site-packages/numpy']
1.1.0
['/home/travis/miniconda/envs/test/lib/python3.5/site-packages/scipy']

So even though numpy 1.13.1 is installed, Python still thinks that 1.15.2 is installed. I'd say that's downright scary.

@weaverba137
Copy link
Member

I pushed an update to the .travis.yml file that fixes the sphinx documentation test at least. I still see the C++ problem with numpy=1.13.3. But there are additional observatoins:

  1. On Python 3.6, the installation is clean, there are no duplicated packages or unplanned downgrades, see: https://travis-ci.org/desihub/desitarget/jobs/451366257#L1198.
  2. On Python 3.5, that is not the case, see: https://travis-ci.org/desihub/desitarget/jobs/451366256#L1273 Even astropy is duplicated!

@tskisner
Copy link
Member

tskisner commented Nov 6, 2018

I was just looking at the log of the last push by @weaverba137 here (https://travis-ci.org/desihub/desitarget/jobs/451366256). Some things that are strange to me:

@sbailey
Copy link
Contributor

sbailey commented Nov 6, 2018

conda install scipy downgrading numpy is common and not previously problematic (e.g. that is also what happens with our desiconda installs).

pip install healpy also installing scipy sounds really bad, and conda install astropy dropping back to pip sounds beyond crazy.

We switched to getting healpy from pip when we started having problems with the openastronomy conda channel. Could we try getting healpy from conda-forge instead?

@tskisner
Copy link
Member

tskisner commented Nov 6, 2018

I believe the --no-binary :all: option to pip will force the c++ compiled extensions in healpy to be built at install time against the current environment. It should also then use the scipy version that is already installed. Is it possible to just add this option when pip installing packages in travis?

Another option: travis provides a plain old virtualenv for every python version in the build matrix. Then we can just pip install everything without conda. Example for 3.6, from another project where I use this technique:

# Install travis python
if [ ! -e "${HOME}/virtualenv/python3.6/bin/activate" ]; then
     wget https://s3.amazonaws.com/travis-python-archives/binaries/ubuntu/14.04/x86_64/python-3.6.tar.bz2
     sudo tar xjf python-3.6.tar.bz2 --directory /
fi
source ${HOME}/virtualenv/python3.6/bin/activate

pip install numpy scipy matplotlib cython astropy ephem healpy cmake

I have had no strange problems with this setup- just saying that maybe we are making things more complicated by using conda in travis...

@moustakas
Copy link
Member Author

moustakas commented Nov 6, 2018 via email

@tskisner
Copy link
Member

tskisner commented Nov 6, 2018

Beware that conda-forge packages can only depend on other conda-forge packages. Depending on the exact details of the conda build file, this may try to bring in other conda-forge versions of numpy / scipy (for example due to the nomkl feature).

@sbailey
Copy link
Contributor

sbailey commented Nov 6, 2018

I'm open to the possibility of refactoring our travis / pip / conda configuration, though something seems wrong about testing with a completely different installation methodology than we recommend using elsewhere.

In the meantime I had good luck switching healpy back to conda with conda-forge in test PR #417, so I'm applying that here rather than diving into a major travis refactor right now.

Whoop; @tskisner just comment that might be a bad idea...

@tskisner
Copy link
Member

tskisner commented Nov 6, 2018

Well, if it "works", then what could go wrong ;-) I guess I'm just advocating keeping things as simple as possible inside the travis environment. If one goal of travis tests is to specifically test the package with anaconda, then not much we can do. If the goal is just to test the package with dependencies from "where ever", then maybe the travis virtualenv plus pip is easier.

@sbailey
Copy link
Contributor

sbailey commented Nov 6, 2018

Let's do whatever is pragmatic to get current failing tests working again, and then discuss the bigger picture plans in desihub/desitemplate#16 . If we are going to switch methodologies, we should do so in a coordinated manner across repos.

There's nothing like reviews and closeout time to bring out the unrelated chaos...

@sbailey sbailey mentioned this pull request Nov 6, 2018
@sbailey
Copy link
Contributor

sbailey commented Nov 6, 2018

Test pass with either conda-forge healpy, or with pip install --no-binary :all: healpy. I'm going to switch the branch to use the pip install method since that it most similar to what we had before (instead of moving nearly all packages to conda-forge as a side effect). For other packages, the trick/solution is to add this to the "install" section near the bottom of .travis.yml so that healpy is installed with --no-binary even if the other pip packages use binaries:

install:
    ...
    - "if [[ $SETUP_CMD == test* ]]; then pip install --no-binary :all: healpy; fi"
    ...

Note the extra quotes, which are necessary so that yaml doesn't interpret the trailing colon on :all: as being yaml dictionary syntax...

@sbailey sbailey merged commit 0128716 into master Nov 7, 2018
@sbailey sbailey deleted the healpix-bug branch November 7, 2018 00:00
qmxp55 pushed a commit to qmxp55/desitarget that referenced this pull request Feb 12, 2020
shuffle indices of mock targets in place
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.

4 participants