-
-
Notifications
You must be signed in to change notification settings - Fork 138
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
Fast build ellipse model #1288
base: main
Are you sure you want to change the base?
Fast build ellipse model #1288
Conversation
Hello @huancho9802! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:
|
It seems like the ReadTheDocs build failed because of my worker.so file (OSError: shared object file not found). Can you help me with packaging the .so file? This is my first time doing this so I really appreciate any help. Thanks! |
Thanks, @huancho9802. @ibusko, can you help with this PR? |
@larrybradley Unfortunately, I don't have much experience with CI, especially with packaging C libraries for ReadTheDocs. It would take me a while to get into that, and I am currently booked solid with JETC work. Next week I go back to work with the Indigo team, maybe we can carve out some time them? |
My memory from a couple of years ago dealing with a (possibly) similar issue is that ReadTheDocs simply won't install Python extension modules (i.e., modules based on compiled C/C++ code). The problem I had was that I had Sphinx generating API-based HTML documentation using Sphinx autodoc commands; this would fail on ReadTheDocs for any module that tried to import the extension module, because the latter wasn't present in the ReadTheDocs installation. (I eventually figured out a kludge where I would generate documentation normally on my laptop, and then repackage the API-based HTML output into "raw HTML" that could be used by Sphinx on ReadTheDocs in place of the autodoc-related commands.) |
Hello all! Thank you so much for taking a look at my PR. I'm aware that the auto build tools fail to build C code and recognize the binary *.so file. I notice that a lot of the C-extensions in the package are written in Cython, so should I migrate my function to Cython instead of pure C? If yes then I will definitely take a look at Cython in the future, though I'm in school right now so it may take a while. For the mean time, you can take a look at the demo at https://github.com/huancho9802/fast-build-ellipse-demo and to see the speedup compared to pure Python code. Thanks! |
isolist : `~photutils.isophote.IsophoteList` instance | ||
The isophote list created by the `~photutils.isophote.Ellipse` | ||
class. | ||
|
||
nthreads: float, optiomal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nthreads: float, optiomal | |
nthreads: float, optional |
isolist : `~photutils.isophote.IsophoteList` instance | ||
The isophote list created by the `~photutils.isophote.Ellipse` | ||
class. | ||
|
||
nthreads: float, optiomal | ||
Number of threads to perform work. Default is 1 (serial code). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Number of threads to perform work. Default is 1 (serial code). | |
Number of threads to execute the task. Default is 1 (serial code). |
Hello! This is a pull request in response to #1168 for a faster build_ellipse_model() function, using C and OpenMP. Changes are
(1) changed data structure for result and weight from 2D numpy array to a 1D C-native array
(2) converted all auxiliary numpy arrays (intens_array, eps_array, etc) into C arrays
(3) ported the for loop into C code
(1) Add new test for a multithreaded version of the function
(2) Add new test for processing a large array (5000x5000), shows that result is returned very fast
Here is the C code for the loop (worker.c):