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

Re-implement binary_search in C #129

Merged
merged 8 commits into from
Apr 23, 2014
Merged

Re-implement binary_search in C #129

merged 8 commits into from
Apr 23, 2014

Conversation

orbitfold
Copy link
Contributor

Here is how I would do it. It works. However there I don't know of a way to raise a Python exception from a pure C function. I can trying doing the others too and C if some kind of speed-up can be achieved.

Please feel free to reject this pull request this is just a proof of concept.

@wkerzendorf
Copy link
Member

@orbitfold great! (I'm CCing @ssim and @mklauser here). I think there are a couple of issues we should find a work around: how do we raise an exception in a C-function? Maybe stackoverflow can help with this.
And how do we check if the code has gotten faster or not.

I think for this we can use a little tool called airspeed velocity asv made by a collaborator of mine. http://spacetelescope.github.io/asv/using.html shows that you can write profiling benchmarks very similar to pytest unit testing frameworks. A final benchmark can be seen here http://mdboom.github.io/astropy-benchmark/. I have not used this tool, but it seems like a good idea. If you have any other suggestions, feel free to try them.

@wkerzendorf
Copy link
Member

ah, and Travis fails, before we could merge this, we would need to change the testing. If you have any questions about the pytest framework let me know.

@orbitfold
Copy link
Contributor Author

http://docs.cython.org/src/userguide/wrapping_CPlusPlus.html

Apparently you can do it in C++ easily, it can catch C++ exceptions and propagate them to Python.

There is even this handy table:

C++                      Python
bad_alloc       MemoryError
bad_cast                TypeError
domain_error        ValueError
invalid_argument ValueError
ios_base::failure  IOError
out_of_range        IndexError
overflow_error     OverflowError
range_error         ArithmeticError
underflow_error   ArithmeticError
(all others)        RuntimeError

http://docs.cython.org/src/userguide/language_basics.html#error-return-values

In C it is possible this way

cdef extern int spam() except -1

So whenever the C function returns -1 it will raise a Python exception, apparently. I cannot test this right now though.

@wkerzendorf
Copy link
Member

I don't know what the best way forward is. I think sticking with C for now seems logical, but can be convinced otherwise. That means that we could use the return -1 formalism.

@orbitfold
Copy link
Contributor Author

Well C++ is (in theory) a superset of C, you can write C code and use C++ features as needed...

@orbitfold
Copy link
Contributor Author

Hmm..

sudo python setup.py install

compiles and installs fine, while

python setup.py build

fails with

Generation of default configuration item failed! Stdout and stderr are shown below.
Stdout:

Stderr:
/usr/lib/python2.7/dist-packages/pkg_resources.py:1031: UserWarning: /home/vytas/.python-eggs is writable by group/others and vulnerable to attack when used with get_resource_filename. Consider a more secure location (set with .set_extraction_path or the PYTHON_EGG_CACHE environment variable).
  warnings.warn(msg, UserWarning)
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/local/lib/python2.7/dist-packages/astropy-0.3.1-py2.7-linux-x86_64.egg/astropy/config/configuration.py", line 654, in generate_all_config_items
    for cfgitem in six.itervalues(get_config_items(nm)):
  File "/usr/local/lib/python2.7/dist-packages/astropy-0.3.1-py2.7-linux-x86_64.egg/astropy/config/configuration.py", line 540, in get_config_items
    __import__(packageormod)
ImportError: dynamic module does not define init function (initcmontecarlo)

running build_scripts

@orbitfold
Copy link
Contributor Author

Finally passed the build and tests!

@wkerzendorf
Copy link
Member

@orbitfold great work. We should merge this as soon as we have asv running - this way we can see if there was any speedup.

@orbitfold
Copy link
Contributor Author

Unless there is something I missed I think this can be merged in.

@wkerzendorf
Copy link
Member

Great! I'm happy with this, but we have a policy that at least two people need to look at it. @ssim or @mklauser can one of you look at this and merge if it's ready.

@ssim
Copy link
Contributor

ssim commented Apr 23, 2014

@orbitfold @wkerzendorf This looks good to me. I will merge it now (and add a line to the change log).

ssim added a commit that referenced this pull request Apr 23, 2014
Re-implement binary_search in C
@ssim ssim merged commit dc2ba8d into tardis-sn:master Apr 23, 2014
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.

3 participants