This was pointed out by @andrew-zentner . ARZ: would you mind describing the source of confusion here in this GitHub issue?

Currently, I use this method defensively_create_directory, or something like that. In the current implementation, if 'nonsense' is passed to this method, an empty directory /.../cache/raw_halos/nonsense/ is created. This is a nuisance, because if a typo is passed to the method, a nonsense directory is created, and never removed. Even worse, the current implementation is dangerous: if this method were called in a loop, an arbitrary number of subdirectories would be created.

The way mock_observables is currently written, the test suite automatically fails if the platform does not have MPI installed. But MPI should be optional. The way to handle this is

• Step 0: Do all development in a virtual environment which does not have MPI installed, so that package-dependency errors such as this are caught from the get-go.
• Step 1: Follow astropy's try/except-based pattern of implementing a global variable HAS_MPI, and add a py.test decorator marker. This is implemented throughout astropy with HAS_SCIPY, HAS_MATPLOTLIB, etc, so that is the place to look to see how to do it.

@duncandc - can you handle this?

If the pair counter is called more than a few times from within a single python session, the session crashes due to "too many files open". The bug traces to the fact that the pools are never closed at the end of the parallelization. The fix is simple: simply add pool.close() whenever the parallel job is complete.

I'll fix this one very soon, @duncandc - but I thought this was worth pointing out.

The package is failing the test suite when merging. This is a astropy problem with the versioning of pytest. @eteq is looking into this for us.

I do not understand the proposed API for this module. The API will depend on the attributes of the mock objects. Let me sketch out my understanding of the mock strucutre as it relates to this module

1.) Mocks will have attributes for all the halo and galaxy props. e.g. mock.mstar will be an array of stellar masses, and mock.halo_id will be an array of halo ID numbers.
2.) For convenience the user can bundle these into astropy tables. One for galaxies and one for haloes. These tables will have keyword access, e.g. mock.gal_table['mstar'].
3.) We don't want those tables to be the default because they are too slow to create.

The aggregation API should be able to do aggregation operations on either an astropy table, or mock object directly. The functions should return the same result in both cases.

Questions:
1.) what should the functions return?
a.) an array
b.) a table with a new column
c.) a mock with a new attribute
2.) Should we internally bundle the needed mock attributes into a table or rec array?
3.) How should the aggregation functions themselves be structured? Right now they operate on slices of record arrays.

Thoughts:
I think the aggregation operations will take time comparable to creating an astropy table (or longer). It may be over optimization to have them be able to run on anything other than a table.

Most scripts in halotools/scripts do not belong in the main repo and should be removed.

For example, both Peter and Alexie's models are based on M200b

cc @duncandc

Easy fix.

@duncandc - I'm using your code to build fast calculators for the PVD. I just noticed that your loops have calls to len() within the range() function, rather than outside the loop ravel. This minor change should not have a significant impact on performance, but there is no reason not to change it.

The analytical randoms are not calculated correctly in clustering.s_mu_tpcf.

I believe the problem is with the volume calculation.

@aphearin @eteq

I get the following error when building a clean clone of halotools once I add a cython module:

"Cython must be installed to build from a git checkout."

I do in fact have cython installed.

Should be arguments for simname, halo-finder, and redshift. Also a -h option that displays helpful usage instructions. Suggested by @surhudm

@duncandc - not sure what the problem is yet, but I'm looking into it. Here's the summary:

test_igraph_functionality fails in my virtualenv. The traceback leads to the following:

      g = igraph.Graph(zip(sources, targets), n=matrix.shape[0], directed=directed,\
                            edge_attrs={'weight': weights},\
                            vertex_attrs={'x':x, 'y':y, 'z':z })

E AttributeError: 'module' object has no attribute 'Graph'

In observalbles.py:
When computing the cross-correlation function using the function 'TP_estimator' we are accounting for the same normalization factor for the two sets of data points (sample1 and sample2).
The right calculation should be:
xi_gm = [D1D2/(N1N2)]/[N_rN_rRR]
instead of
xi_gm = [D1D2/(N1N1)]/[N_rN_rRR]

The best thing to do will be to add factor3 = (len(sample1)0.5)*((len(sample2)0.5))/len(randoms) and to pass this factor to 'TP_estimator' when computing the cross-correlation function.

This bug has to be fixed in both:

1. two_point_correlation_function
2. two_point_correlation_function_jackknife

Shany

The TPCF functions should throw a warning when there are no random pairs in a radial bin. As of now, they continue ahead and return 'inf'. This can be confusing to the user.

The halotools/configuration.py module has a function that is used in the read_nbody module to identify the CATALOGS directory where the halo catalogs are contained. This function behaves properly in all cases except when running the testing suite, evidently because the py.test framework temporarily moves the code to another "clean" directory before running the test suite.

I think an is_monotonic() function would be useful. What do you think @aphearin ?

@eteq - could you confirm or deny this? The only thing you need to do is just execute the full test suite after checking out bd035cb, which is part of the sim_manager_revamp branch on my personal Halotools fork.

(Do you know a ProTip for how I can provide a GitHub reference to a specific branch, e.g., a symbolic link to " the sim_manager_revamp branch on my personal Halotools fork"?)

In various places, the scipy.interpolate.interp1d function is used. Since this function is written in pure python, it is much slower than scipy.interpolate.InterpolatedUnivariateSpline which is a wrapper around Fitpack.

A benchmark of the functions is discussed here: http://www.shocksolution.com/2009/04/lookup-tables-and-spline-fitting-in-python/

This is easily managed. The default location is a subdirectory of the astropy home directory. Users may wish to store large halo catalogs on a separate disk, so just add a default_catalog_dir to the configuration module. If None (the default setting in the package), use astropy home. If not None, use the explicit dirname provided there. Whenever loading catalogs, check the state of this variable first.

Mostly this will only impact the halo catalog and merger tree dtype formatting.

Thanks for the tip, @eteq - I'm on it.

This problem was pointed out by @andrew-zentner . The use of retrieve_haloprops is obfuscating, and leads to spaghetti code. Replace all usages with **kwargs syntax.

See if creating a local variable for grid2.slice_array[icell2] in the _npairs_engine saves memory (and therefore speeds up to avoid cache misses)

@eteq - I know you have been consulting with @duncandc over the past couple of weeks about cythonization. There is some new and strange phenomenology that I'm wondering if you've encountered before. This may be more efficient to discuss in person, I don't know. But in the current version of master (which has a test failure - see #96 ), I'm no longer able to test specific modules. I can run the entire test suite at once, but if I try "python setup.py test -t fname", I get the following error:

error: [Errno 2] Could not find C file halotools/mock_observables/pair_counters/cpairs/cpairs.c for Cython file halotools/mock_observables/pair_counters/cpairs/cpairs.pyx when building extension halotools.mock_observables.pair_counters.cpairs.cpairs. Cython must be installed to build from a git checkout.: 'halotools/mock_observables/pair_counters/cpairs/cpairs.c'

I strongly suspect this is related to conventions we have discussed previously in #69 .

Satellite positions in the Kravtsov04 model are not bounded by Lbox, as pointed out by @duncandc

This is worth being aware of, @andrew-zentner - I'll fix it soon.

Good suggestion, @duncandc , and easy to implement. I'm on it.

When I populate two mocks with the Hearin15 model, one with a threshold of 10.25, and the other with a satellite fraction of 11.25, the satellite fractions are both ~0.3.

As @aphearin noted in #20, some of the travis builds are still failing with an error about TypeError: run_tests() got an unexpected keyword argument 'repeat'

In fact, this is what d62c292 fixed by switching most of the builds to use numpy 1.9 . The underlying problem was that the astropy version installed by conda is only available for numpy < 1.9 as astropy v0.4.1 (which is apparently incompatible due to this bug you noticed). But those last three builds are there specifically to check versions of numpy < 1.9 ... which of course now fail.

At some point the plan is to fix astropy_helpers to make this work again... But until then these builds will fail. Perhaps the best thing is to just comment those out for now and put them back in when/if astropy_helpers is updated?

In the current version of the repo, the test suite passes, but the mock_observables sub-package fails to import. This seems to trace to ckdtree, which I thought we no longer were going to use, but I'm not sure what other routines may still be dependent on ckdtree.

I think, but am not sure, that this is what is causing the ReadTheDocs build of the documentation to fail. So until @duncandc has a chance to try and fix this, then @eteq should ignore the RTD build problem.

I am getting strange mis-matching between the number of columns in a multidark high-z ASCII data. Still not sure what's causing this. Could also be a problem for other catalogs.

We should create a very simple interface for running a no-frills TPCF on mocks w/ PBCs.

astropy doesn't automatically install.

@duncandc - With the addition of the group aggregation calculation, it is finally an issue that we need to start using a consistent capitalization convention. The same convention should be adopted in the groups keys and the halos keys.

In particular, under what conditions should a key name contain a capitalized letter? I have been using no capital letters for any halo key, e.g., 'id' denotes the halo ID, and 'vmax' denotes V_{\rm max}.

I know this seems trivial, but this is the sort of that we should decide on now rather than wait until it becomes a bug, because it will become a bug with 100% certainty, and there will be far more code to write in the future than now.

Implement a snapshot.host_halos @Property. This returns a table of the host halos as if it is a property. Extremely useful for quick checks, as it eliminates the need to construct a UPID mask.

Currently, HodFactory has kind of a clunky way of passing the correct arrays to each component model. The retrieve_inherited_behavior method only permits one or two arguments to the component models, and these two arguments are always presumed to be prim_haloprop, and prim_haloprop+sec_haloprop, respectively.

This awkwardness and rigidity is easily avoided. Just have require that component models get instantiated with a tuple of strings that are the arguments needed by the critical method of the component model.

More generally, there should be a separate module that provides common mappings between halos and halo profile parameters. A concentration-mass relation is just one example. Another example pertaining to a generalized NFW profile would be a mapping between the steepness of a cusp and halo mass.

This module governs separate behavior from the actual shape of an NFW, Einasto, etc., and so should be kept separate from the module that provides the mappings between halo radius and halo mass density. This module should be called something like halo_prof_param_components, whereas the latter should be (is already) called halo_prof_components. Classes defined in these two modules are obviously closely associated. In particular, the constructor of a model defined in halo_prof_components (e.g., an NFW profile), will take as input a model object defined in halo_prof_param_components (e.g., a concentration-mass relation).

Instances of OccupationComponent have a hidden .publications requirement, which is a bug.

Include a composite model option for a user-defined function that can be used to produce a new halo property as a primary or secondary halo property. Implement in mock_factory by calling this model component during the process_halo_catalog method.

@aphearin Do we want to have a separate function to calculate a simple TPCF and the TPCF with jackknife errors?

options:
1.) separate functions (tpcf, tpcf_jackknife)
2.) only calculate TPCFs with errors
3.) one function with a complicated API (and code) that has it as an option.

Suggested by @surhudm

@duncandc - I just updated my master branch with your latest changes. Did you run the test suite before submitting your PR? When I run the test suite, not only are new errors produced, but in fact, the entire execution of the test suite just fails and the system exits before the suite is done running.

@eteq - how can such a thing happen, anyway?

These are naked, but should have some attrs set for verification purposes.

For example, a component model may simultaneously return a disc scale length and a bulge-to-disc mass ratio.

For reasons I have not been able to track down yet, the Hearin15 composite model is returning the result that the fractional impact on clustering of assembly bias has no dependence on the amount of underlying scatter, which cannot be correct. In particular, what I compute is the following. I calculate clustering for a model without assembly bias, and for a model with assembly bias, by toggling the central_assembias=True/False and satellite_assembias=True/False in the Hearin15 kwargs. I compute this ratio for scatter 0.2 (default), and for scatter 0.8. I find that the clustering ratio is identical.

One bug I know of for sure is with the satellites. When I directly compute the mean occupations for satellites, I find it has zero dependence on the scatter. This means that the scatter param_dict is not propagating down to AssembiasLeauthaudSats.

However, here is a truly strange thing. When I use this model to compute mean and mc occupation statistics of centrals, I see the expected behavior with changes to scatter. I see that increasing scatter boosts the low-mass occupations, and it also increases the dynamic range over which low- vs. high- concentration halos receive boosted occupations. But bizarrely, when I compute the clustering ratios using the include_crosscorr mechanism, I see zero difference for central-central clustering. I do not see how this can be.

First things first, fix the known satellite bug, and then turn attention to the centrals.

The problem mentioned in #17 is now mostly fixed (a few of the builds still have it, but a fix is in the works for that from the astropy_helpers side). But now that the tests are actually running on travis, a different problem has showed up. To see an example, take a look at one of the failing builds from https://travis-ci.org/astropy/halotools/builds/49241697 . There, you'll see a bunch of errors like ImportError: No module named scipy.special.

What's happening there is that the default builds do not include scipy. This is because, in astropy, we have a general guideline that scipy is not a strict requirement, but rather it is necessary for certain things (like model fitting) that use scipy. So any test that invokes functionality requiring scipy is skipped if scipy is not installed, but run if it is. For an example of this, see https://github.com/astropy/astropy/blob/v1.0rc1/astropy/modeling/tests/test_fitters.py - you'll see a block at the top that defines the HAS_SCIPY variable, and then tests below that require scipy have decorators that look like @pytest.mark.skipif('not HAS_SCIPY').

As it stands, however, even that will not work for halotools - this is because there are at least some places where scipy is imported at the top level. (E.g., https://github.com/astropy/halotools/blob/master/halotools/halo_occupation.py imports a variety of things from scipy). In astropy, the convention (where possible) is to have scipy imports always inside a function/method, so that the error only happens when that functionality is used, rather than when the package is imported. So then stuff that requires scipy fails, but everything else can still succeed.

So there are two different solutions:

1. Update halotools both to skip the scipy-requiring tests and move all scipy imports inside functions.
2. Just take the stance that scipy is a dependency of halotools. setup.py should updated to add scipy to the requires list, and travis can be updated so that scipy is always installed (both of those are pretty much one-line changes).

I can set up option 2 myself if thats the way you want to go, @aphearin, but of course someone more familiar with halotools would have to do option 1.

=================================== FAILURES ===================================
[doctest] halotools.empirical_models.sfr_components.BinaryGalpropInterpolModel.init
192 and 90% for massive cluster centrals (:math:M_{\\mathrm{vir}}=10^{15}M_{\odot}).
193 We can use the BinaryGalpropInterpolModel to implement this as follows:
194
195 >>> abcissa, ordinates = [12, 15], [1/3., 0.9]
196 >>> cen_quiescent_model = BinaryGalpropInterpolModel(galprop_key='quiescent', abcissa=abcissa, ordinates=ordinates, prim_haloprop_key='mvir', gal_type='cens')
197
198 The cen_quiescent_model has a built-in method that computes the quiescent fraction
199 as a function of mass:
200
201 >>> quiescent_frac = cen_quiescent_model.mean_quiescent_fraction(prim_haloprop=1e12)
UNEXPECTED EXCEPTION: IndexError("0-d arrays can't be indexed.",)
Traceback (most recent call last):

File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/doctest.py", line 1289, in __run
compileflags, 1) in test.globs

File "<doctest halotools.empirical_models.sfr_components.BinaryGalpropInterpolModel.init[2]>", line 1, in

File "halotools/empirical_models/sfr_components.py", line 326, in _mean_galprop_fraction
mean_galprop_fraction[mean_galprop_fraction<0]=0

IndexError: 0-d arrays can't be indexed.

halotools/empirical_models/sfr_components.py:201: UnexpectedException
______________________________ test_unbiased_nfw _______________________________

def test_unbiased_nfw():
    nfw_prof = hpc.NFWProfile()
    gal_type = 'satellites'

    sat_prof = gpf.GalProfFactory(gal_type, nfw_prof)

    snapshot = FakeSim()
    composite_model = Kravtsov04()
    mock = HodMockFactory(snapshot=snapshot, model=composite_model)

    # Check that mc_radii gives reasonable results for FakeSim
    satellite_boolean = mock.galaxy_table['gal_type'] == gal_type
    conc_key = mock.model.gal_prof_param_list[0]
    satellite_conc = mock.galaxy_table[conc_key][satellite_boolean]
    satellite_radii = sat_prof.mc_radii(satellite_conc)
    assert np.all(satellite_radii < 1)
    assert np.all(satellite_radii > 0)
    # Check that mc_radii scales properly for high- and low-concentrations
    conc_array_ones = np.ones_like(satellite_conc)
    conc_array_tens = np.ones_like(satellite_conc)*10
    high_conc_radii = sat_prof.mc_radii(conc_array_tens)
    low_conc_radii = sat_prof.mc_radii(conc_array_ones)
    assert high_conc_radii.mean() < satellite_radii.mean() < low_conc_radii.mean()

    # verify that all mc_angles points are on the unit sphere
    x, y, z = sat_prof.mc_angles(1000)
    unit_sphere_pts = np.array([x, y, z]).T

  norms = np.linalg.norm(unit_sphere_pts, axis=1)

E TypeError: norm() got an unexpected keyword argument 'axis'

halotools/empirical_models/test_empirical_models/test_gal_prof_factory.py:74: TypeError
===================== 2 failed, 71 passed in 12.64 seconds =====================

Suggested @duncandc