Subhalo-based phase space model for satellites - #637

@eteq and @bsipocz - this is quite a mysterious test failure that only occurs for astropy development test runs. I'm at a loss what this test has to do with anything on the helpers-side. Any ideas?

@aphearin - I've tried to investigate but the building process dies earlier, and the tests don't even start up. I'll try to look at it again tomorrow.

Thanks @bsipocz. Very briefly, for context, this is an integration test in which I create a Monte Carlo realization of a galaxy population, where the population is stored in an Astropy Table called galaxy_table. I then assert that some of the galaxies are red, others are blue. That test passes in all but the astropy-dev builds, which is perplexing to me.

Something must have changed in the last two months since the last tests run on halotools ;) Joke aside, I'll have a look at it tomorrow.

Also for the long term, I think it will be useful to have a cron job travis (which is a new thing) run ~weekly for halotools, so these failures are spotted sooner. I'm testing some enhancements with ci-helpers, so it will also be possible just to run a few builds that are using dependencies that can change rapidly, e.g. the ones with astropy or numpy dev. I'll keep you posted.

I think that's a good idea. My own development cycle will always be intermittent, since I switch back and forth between implementing new features and then using them for science. Let me know if I can help with implementing the travis cron job.

@aphearin - Could you restart the failing travis builds? Locally, using astropy dev, everything works for me, all the tests pass, so probably there was some last minute merge for 1.3 that did the trick.

Also, $EVENT_TYPE was introduced as an env variable for ci-helpers, so you can define builds to run only in for certain job types, e.g. only from cron, or for merge/pushes or for PRs, etc. I'm not sure how useful it is for halotools, but maybe it is.

@aphearin - My apologies, I though I was testing with latest master, but I was rather on one of my feature branches (branched from a6dcf8b87c89b6e7c8d54001181dba7c32056a52). Now with latest master I see the same error. Meaning that the regression is coming from one of these pull requests:

    Merge pull request #5576 from eteq/update-branching-instructions
    Merge pull request #5552 from dhomeier/fortranexp2
    Merge pull request #5569 from mhvk/omit-color-cycle-warnings
    Merge pull request #5535 from parejkoj/luptonRGB
    Merge pull request #5568 from bsipocz/travis_sphinx_version
    Merge pull request #5245 from mhvk/quantity-from-string
    Merge pull request #5567 from eteq/fix-conf-plot-tuple
    Merge pull request #5564 from bsipocz/extern_update_js_3.1.1
    Merge pull request #5561 from eteq/add-exc-for-empty-catalogs
    Merge pull request #5556 from taldcroft/table-replace
    Merge pull request #5486 from taldcroft/yaml-serialize
    Merge pull request #5565 from mhvk/numpy-dev-boolean-index-failure
    Merge pull request #5554 from eteq/make-conv-ndd
    Merge pull request #5562 from saimn/fix-table-show_in_browser
    Merge pull request #5560 from bsipocz/fits_rename_pyfits_modes
    Merge pull request #5505 from Cadair/runner_subclassing

I'm almost 100% sure that the regression is coming from here astropy/astropy#5556

That does indeed look like a very likely candidate, @bsipocz. The potentially "problematic" example given by @taldcroft at the end of astropy/astropy#5556 looks extremely similar to the form of the statement that fails in this PR.

@aphearin - from the test failure report I can't quite see the part of your code that was affected by #5556. I am worried about these kind of regressions so any explanation would be appreciated.

@taldcroft - I'm not positive that #5556 is responsible, but it looks quite similar. The assertion in my test suite that fails is the following:
assert set(model_instance.mock.galaxy_table['quiescent']) == {True, False}
This is line 138 of tests/docs_code_block_tests/test_hod_model_factory_tutorial.py. What this test does is first create a Monte Carlo realization of a galaxy population, stored in the form of an Astropy Table. One of the columns stores a boolean-valued quiescent attribute, specifying whether the galaxy is actively forming stars. The model instance should definitely have both True and False values. Nothing in this PR touches code that should affect this, and this test passes in all test suite executions except the Astropy-dev runs.

Does that help? Let me know if I can provide more context or answer other specifics.

@aphearin - Yesterday I went through those PR merges on astropy dev. The first point when the fail appeared was for 5556, so I'm quite positive it's the source of it.
I've tried to find and fix halotools by looking at modifications of Table objects, but gave up after about 30 mins, as none of the line I suspected would fix the test. I think someone with better knowledge of the codebase and the expected behaviour will eventually succeed.

Ok @bsipocz - can you maybe briefly describe what specific change to the Table behavior I should be aware of so that I can make the modifications? When you were going through the code, what sorts of changes were you trying out? From the description on 5556 it was not clear to me, especially since this change only impacted a single line of code in the entire repo.

@aphearin - what you should be looking for are places where you update table column data in place. That is the only change, where something like table['a'] = values will now entirely replace the column (with effectively Column(values)) instead of an in-place update. This will drop any existing metadata. If there are references to table['a'] floating around in your code then they will be invalid. The easy fix is using table['a'][:] = values to force an in-place update.

So we were concerned about merging #5556, and if it turns out that subtle problems are common then I might consider backing it out before the release. So your investigation now is especially timely and helpful. I think that on the whole #5556 is a good thing (e.g. pandas behaves this way), but we don't want to cause tricky breakage problems.

@aphearin - I were exactly looking for cases @taldcroft mentions, where you update columns of e.g. galaxy_table in halotools/empirical_models/factories/*py files. Changing a few cases to specify the slice [:] didn't solve the issue, and didn't have time to do a proper debugging.

I'll look into this asap, @bsipocz and @taldcroft. I'm quite concerned, actually. It sounds like a pretty serious undertaking that will affect hundreds of lines distributed throughout the code base in a way that cannot be fixed in an automated fashion.

So I was able to resolve the bug using the syntax change suggested by @taldcroft - but I'm concerned I just got lucky as there are many, many places throughout the repo where things could have gone wrong. What concerns me even more is how "silent" this Table-related bug is - users building their own Halotools models will have to be aware of this specific in-place syntax or they will get crazy results that do not raise exceptions. So FYI, I'm going to leave this open for the time being while I revisit the many places this sort of problem may occur.

@aphearin - in this case is the problem related to lost meta data or a lost reference? Those are the only two reasons I'm aware of that a problem could arise.

In looking at the code I can't immediately see why there was a failure in the test. In any case I appreciate that this could be confusing... grr.

One thing that might be useful is to insert statements to print the column before and after the update using the original syntax (without [:]) and new syntax and put that in here. I just want to see what is going on and why this is creating a problem. That will help understand if things like this can come up all the time. If I get a little more time I suppose I could just install myself...

@taldcroft - I still don't understand why the tests fail either, partly because I myself have not yet installed the dev-branch of astropy and done inspections like the one you suggest.

Here's a high-level explanation of what is going on in the code. There is a container class (the HodModelFactory) that is responsible for collecting a set of functions defined in independently-written component classes. The container class is responsible for passing the appropriate Table slice to each component class. So, in the example here, the _mc_galprop function is a method of the BinaryGalpropModel component class that I patched; the HodModelFactory container class passes the _mc_galprop function the table keyword argument, which is a pointer to the slice of the galaxy_table that stores information about central galaxies. On the last line of _mc_galprop, the one I changed in b28fe15, kwargs['table'][self.galprop_name][:] = result, you can see that there is an existing quiescent column of the galaxy_table that has been previously initialized to np.zeros(num_total_gals, dtype=bool), and in this line the initialized values are being overwritten with values defined by the model. For reasons that are still unclear to me, if I drop the [:] syntax, the values of the quiescent table column are never updated with the values assigned by the _mc_galprop function. I wish I understood why that is happening, but in any case, that is the problem, and it's solved by the [:] in-place syntax.

I installed halotools and did some playing around with the code itself. I can't follow the logic between the code where you added [:] related to kwargs['table'] and the test that fails. My inference from the test is that the True and False values set in that method should be occurring in the model_instance.mock.galaxy_table['quiescent'] columns. BUT the id of model_instance.mock.galaxy_table is different from kwargs['table'] (and in fact the lengths are different) so I can't tell what is going on.

The problems would make sense if there was a table slice happening at some point to create in-memory references of columns that subsequently get broken. For example, before #5556:

In [8]: t = simple_table()
In [9]: t2 = t[:2]
In [10]: t2['a'] = [100, 100]
In [11]: t
Out[11]: 
<Table length=3>
  a      b     c  
int64 float64 str1
----- ------- ----
  100     1.0    c
  100     2.0    d
    3     3.0    e

After #5556 the value of t['a'] is unaffected when t2 gets updated because it just makes a new column instead of doing an in-place update of the shared memory between t['a'] and t2['a']. So is your code doing anything like that?

Yes @taldcroft, that is precisely what is going on. A slice of the galaxy_table is created and passed to the function, then the values in the slice are (should be) over-written. This is exactly what is going on, nice detective work.