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

added gird_pairs pair counter and tpcf code #69

Closed
wants to merge 21 commits into from
Closed

added gird_pairs pair counter and tpcf code #69

wants to merge 21 commits into from

Conversation

duncandc
Copy link
Contributor

@duncandc duncandc commented Jun 3, 2015

compiles, passes tests, and runs on my machine.

Bolshoi like tpcf calcs take ~5 seconds.

@aphearin
Copy link
Contributor

aphearin commented Jun 3, 2015

The test suite passes on my machine as well. However, the sub-package does not import. In particular:
$ python setup.py build
>>> from halotools import mock_observables

ImportError: No module named grid_pairs

@eteq - why would this be? The cython code seems to be compiling just fine after building it.

@aphearin
Copy link
Contributor

aphearin commented Jun 4, 2015

@duncandc - do you have this same problem? If you do the following:
$ git clean -dfx
$ python setup.py build
>>> from halotools import mock_observables

Does your sub-package import?

@duncandc
Copy link
Contributor Author

duncandc commented Jun 4, 2015

This is because the setup_package.py script does not build it in place. You have to either cd into the build directory, or go into mock_observables/pair_counters/grid_pairs/ and type: python setup_package.py build_ext --inplace

@aphearin
Copy link
Contributor

aphearin commented Jun 4, 2015

@eteq - is there not a workaround to this? That's kind of a pain in the neck to ask our users to do.

@duncandc
Copy link
Contributor Author

duncandc commented Jun 4, 2015

This is annoying... Regular users wouldn't need to to this right? When they install halotools, they will be working from a 'built' version. Could we add a script to the setup.py file which builds the cython utilities in place?

@duncandc
Copy link
Contributor Author

duncandc commented Jun 4, 2015

Also, no need to merge this. I already have an updated version.

@aphearin
Copy link
Contributor

aphearin commented Jun 4, 2015

Yup, agreed. This is too annoying to ask of regular users. It seems like the astropy package-template must have solved this compile problem, so let's just wait to hear what @eteq says.

I'll keep this PR open for discussion purposes only. We can close it when we get to the bottom of this.

Otherwise, the code looks pretty damn good - nice job.

@aphearin
Copy link
Contributor

aphearin commented Jun 8, 2015

Ok, @eteq - here is the summary of the status. From a clean version of the duncandc/mock_obs_devel branch (i.e., after running $ git clean -dfx), the code builds just fine with python setup.py build. And the test suite then runs just fine. However, when I attempt to import mock_observables, there is an ImportError.

I confirm the comment made by @duncandc above - if you just cd into mock_observables/pair_counters/grid_pairs and run $python setup_package.py build_ext --inplace, then that compiles just fine and the sub-package now imports.

How can we fix this so that the original, package-wide call to build the code from root correctly compiles all sub-package cython modules?

@duncandc
Copy link
Contributor Author

duncandc commented Jun 8, 2015

@aphearin and @eteq, so what's the deal? I'm pretty bored over here, and could probably get something done...

@eteq
Copy link
Member

eteq commented Jun 13, 2015

Will check this out ASAP, hopefully tomorrow (for some reason this did trigger my notification e-mails like it normally does. Probably something I did...)

@eteq
Copy link
Member

eteq commented Jun 14, 2015

@duncandc @aphearin - I can't reproduce the problem. Here's what I do:

  1. checkout a fresh testing branch
  2. git reset --hard master - resets the branch to be exactly the same as master
  3. git merge duncandc/mock_obs_devel - merges all of this branch's changes
  4. python setup.py build
  5. cd build/lib.macosx-10.10-x86_64-2.7
  6. start python
  7. from halotools import mock_observables
    and it imports without any problem. This should reproduce exactly what users will see if they were to install using pip or python setup.py install, so if that probably the most important thing.

Alternatively, If I start from 4 and instead do python setup.py build_ext --inplace and then run python and do from halotools import mock_observables it still imports without complaint.

I almost never use --inplace because it makes it easier to change the source code and then forget to re-compile (because the python code doesn't need to be recompiled), so I'm not 100% sure that's always reliable... But it did work for me just now.

@aphearin
Copy link
Contributor

@eteq - great, thanks for checking on this.

First of all, I think all three of us confirm that your first sequence of operations works just fine. The problem was never something we were worried about for users, sorry if that wasn't clear. The problem was the nuisance of being a developer of a module that used cython.

So, suppose I was working on the pair-counting module, and I was making changes all the time, and with each change I was re-running the test suite. After each round of changes, what Duncan and I were doing was simply navigating to the root of the working directory, running $python setup.py build, and then opening up a python terminal from root and trying to import the mock_observables sub-package. This throws an error, because we were not using the --inplace flag. Neither of us knew about this. I think this solves the problem, though I will wait for @duncandc to confirm.

So now the question is just a matter of what workflow you recommend in such cases. When you're working on a section of Astropy in which you're heavily editing cython code, how do you like to operate? Do you just run $python setup.py build, and then cd into the build directory? Can you explain why you never use --inplace, I didn't quite follow your word of caution.

Anyway, I think now we know how to proceed, so @duncandc - just close this Issue if you agree that this is no longer a roadblock. But @eteq , it would be useful to get your advice on the day-to-day nuts and bolts developing a cython module since we'll be doing this a lot, and I assume that by now you must have an effective workflow for this.

@eteq
Copy link
Member

eteq commented Jun 15, 2015

Aha, I gotcha now, @aphearin. My usual workflow when doing something cython-y where I want to play with it interactively is to have two terminals (or two tabs, usually): one is set to the root of the repo, and the other I cd into build/lib<whater> and start ipython. When I make a change I want to see, I do python setup.py build in the first terminal and then do a reload(...) as necessary in the ipython session (or more often just exit and restart ipython because it's easier to be sure you didn't forget to reload something that way).

An alternative workflow is to use tests instead of an interactive session (if the particular work can be well-phrased as a quickly-running test). ``python setup.py test --args="-k test_function_name" can be used to trigger a single test, and that way I don't have to come back and write a test later. I even wrote a sublime text build system to help automate that process.

As for why not to use --inplace, honestly I'm not sure it matters that much verses the first workflow I mentioned. Occasionally --inplace gets confused and doesn't re-compile when you are jumping around between branches, or a few other situations I've not really entirely understood the cause of, requiring a git clean -dfx instead of an rm -rf build. And if you're doing stuff with package data, using the source instead of the build directory might sometimes result in different paths or files not copied over to the build directory. But none of that's really too big of a deal as long as your test suite checks these things. I think I just like the "cleaness" of testing on the same thing the final resulting package will give, but it probably isn't really objectively that much better.

@duncandc
Copy link
Contributor Author

@aphearin I'm still working on an update to this module. If needed, the this pull request can be merged and used, and I will update it ASAP.

@duncandc duncandc closed this Jun 23, 2015
@aphearin aphearin mentioned this pull request Jul 13, 2015
eteq added a commit to eteq/halotools that referenced this pull request Jul 15, 2015
…ting

Fix CSS formatting of "Other Parameters" section
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants