Articles on tirthasheshpatel's Blog

Final Week: PR Merged 🎉

tirthasheshpatel@gmail.com (tirthasheshpatel) — Sun, 22 Aug 2021 15:46:49 +0000

What did you do this week?

This was the final week for GSoC and we (somehow) managed to get my PR merged! It broke the Gitpod integration but it was immediately fixed afterwards in scipy#14603. I also wrote and submitted my final report as my final evaluation.

As per my proposal, I was able to achieve all the listed milestones. Although I was able to add two new generators from UNU.RAN, there are a lot more which would be good to have in SciPy and would improve the performance significantly (e.g. NumericalInversePolynomial and AutomaticRatioOfUniforms). I have filed an issue scipy#14600 with a list of generators that can be added to SciPy over time. I am willing to continue the work on this project after GSoC and hopefully resolve the issue. I have started to work on adding NumericalInversePolynomial in scipy#14619 and already received some reviews from one of my mentors! I hope to add at least the most important generators before the 1.8.0 release.

I am happy I got a chance to work on such an interesting project and was able to achieve my goals. As always, my mentors and many other developers supported the project and contributed their expertise to increase the quality of my work. The output was orders of magnitude better than what I initially proposed. Thanks to everyone involved in this project!

Week #10: Finishing up the PR

tirthasheshpatel@gmail.com (tirthasheshpatel) — Sun, 15 Aug 2021 04:16:25 +0000

What did you do this week?

Last week I got stuck at some failures on Linux Python3.7-dbg (debug builds). I was able to replicate it locally and thankfully also found a fix for it. Turns out, I was accessing a Python attribute while a live exception was set in Python. This is not allowed because the Python objects of the class might have been destroyed or garbage collected and my attribute lookup might fail with a segmentation fault. This was caught by the debug build of Python 3.7 and was fairly simple to fix: just removed the offending line and the tests passed again! To ensure this doesn't happen again, I refactored some code to make more careful use of PyErr_Occurred().

There was also some discussion around what to name the seed parameter: the new Generator API favors something other than random_state which has been well-established in SciPy. As there was no consensus among what to name it - some favored consistency while others wanted to differ between the new and the old API - we decided to go with the plain old random_state and change its name in some other PR at all places in SciPy.

What is coming up next?

Almost all the reviews have now been addressed, the CI is green, and I am happy with the current state of the PR. It seems like a good starting point and if things stay this way, we might merge by the end of this week. Once that's done, adding new methods is fairly straightforward. I will try to add as many methods as possible before the end of GSoC.

Did you get stuck anywhere?

No blockers this week.

Week #9: CI failure on Python3.7-dbg

tirthasheshpatel@gmail.com (tirthasheshpatel) — Mon, 09 Aug 2021 11:47:22 +0000

What did you do this week?

I addressed more reviews this week and added pickling support to all the generators. Most of the changes focused on the documentation, tutorials, and the UI. We removed the params keyword (used to pass positional arguments to the passed callable) from the method constructor and had a discussion on renaming the seed parameter to something like numpy_rng. I also added some histogram plots to the docs and tutorials. Lastly, as all the generators are Cython extensions classes with a non-trivial __cinit__ method, Cython was not able to generate code to pickle object automatically. Hence, I added a rudimentary __reduce__ method which enables pickling support. Relevant tests have also been added.

What is coming up next?

We need to finalize renaming seed to something more apposite like rng or numpy_rng. All other blocking comments have been addressed, except a recent failure on CI. I will investigate the cause of the failure and try to resolve it by the end of this week. As the GSoC deadline is imminent, I hope to complete that by next week so we could merge the PR.

Did you get stuck anywhere?

I noticed a few failures on the linux workflows on Python3.7-dbg build. For some reason, Python crashes when TransformedDensityRejection is called with a invalid PDF. It seems like there is some internal logic error in the C/Cython code. As this is not a Python error with a proper traceback, it makes it very difficult to even locate the error, let alone solving it! I am thankfully able to replicate it locally and hope to figure out the failures as soon as possible.

Week #8: Addressing Reviews

tirthasheshpatel@gmail.com (tirthasheshpatel) — Thu, 05 Aug 2021 01:49:51 +0000

What did you do this week?

Sorry, I am late publishing this blog! I will resume my regular schedule next week. I spent this week addressing the reviews on my PR. There were comments on the documentation suite and tutorials on formatting issues, obscure phrasing, and naming conventions. As we are getting near the deadline, we decided to leave larger enhancements to docs and tutorials to a follow-up PR. Only a few comments are left to be addressed which I hope to complete by the next week. Also, Nicholas reviewed the C/Cython code again and approved those changes.

What is coming up next?

As GSoC deadline is approaching, we have also decided to try and merge the PR early in the week of August 8 so that more UNU.RAN methods can be added in these last few weeks of the program. I also want to clean up the C/Cython code and go through the docs and tests again to resolve typos and grammatical errors, if any. There are two approvals on the PR already so, hopefully, we could achieve this deadline.

Did you get stuck anywhere?

No.

Week #7: More Reviews on my PR

tirthasheshpatel@gmail.com (tirthasheshpatel) — Mon, 26 Jul 2021 11:25:35 +0000

What did you do this week?

I got a few more reviews on my PR last week. It engendered a interesting discussion on the level of detail we should add in the tutorials. There a few subtle concepts that need to be elucidated for general users to adopt the new API easily. Christoph committed a draft with new description to help explain those concepts better. There also also some other discussions on what to name of the method to sample random numbers from and how to point the users to the new Generator API. As I have an exam coming up next week (details in next section), I was a little quite this week.

What is comping up next?

I will be addressing the reviews on my PR. I also want to benchmark some methods to sample from discrete distribution sometime later. I have my GRE examinations next week on 2'nd August (Monday) so this week might be more silence from my side :) But I will resume my regular schedule right after the exam.

Did you get stuck anywhere?

No.

Week #6: More Benchmarks

tirthasheshpatel@gmail.com (tirthasheshpatel) — Sun, 18 Jul 2021 12:31:03 +0000

What did you do this week?

I continued writing benchmarks for UNU.RAN methods to sample from continuous distributions. The results of these benchmarks are quite promising. They outperform the default rvs method in SciPy on an average by 100x better performance. In some cases, when the PDF/CDF are expensive to evaluate, UNU.RAN methods are 10000 times faster than the default rvs method. It would a nice future project to add a specialized rvs methods for some distributions where UNU.RAN performs significantly better than SciPy. For more details and some pretty plots, please look at tirthasheshpatel#10. With the help of Ralf Gommers, UNU.RAN submodule got transfered under the SciPy organization this week. The new submodule is now present at scipy/unuran.

What is coming up next?

I am hoping to get some reviews on my PR over the period of next week. Other than that, I don't have anything specific to do this week. I will maybe benchmark UNU.RAN method to sample from discrete distributions...

Did you get stuck anywhere?

Nope. Thankfully, another smooth week.

Week #5: First Phase Ends...

tirthasheshpatel@gmail.com (tirthasheshpatel) — Mon, 12 Jul 2021 04:31:17 +0000

What did you do this week?

The PR resolving the memory leaks is still up for reviews. While it was being reviewed, I thought it would be nice to think ahead of time and look at the potential performance of the methods I propose to add. So, I created a new branch gsoc-unuran-bench on my fork and started wrapping the remaining methods I had proposed in the excel sheet that I wrote a couple of weeks ago. I then wrote a small Python script to benchmark all the wrapped methods against NumPy random number generators. For now, I have only used two distributions: Standard Normal and Beta(2, 3). I plan to add more in the following weeks. Sampling was run 3 times per measurement. The results of the benchmark:

UNU.RAN's methods (namely NumericalInversePolynomial and AutomaticRatioOfUniforms) were 3x faster than the NumPy RNG for the Beta(2, 3) distribution.
NumPy RNG was slightly faster than UNU.RAN's methods (with NumericalInversePolynomial and AutomaticRatioOfUniforms being the closest to the performance of the NumPy RNG) to sample from the Standard Normal distribution.

It is good to see that there is a possibility of improving the performance of sampling from some distributions once the methods from UNU.RAN are integrated in SciPy.

What is coming up next?

There are already some reviews on the PR resolving memory leaks and I hope by the end of the next week, there would be even more and we could decide whether we want to use that approach in SciPy. It's a tricky and non-conventional approach so I am not sure how many reviews would be considered "enough" or how much time will it take for the maintainers to properly review it. But while that is going on, I hope to start wrapping methods to sample from discrete distributions and benchmark them against the NumPy RNG.

Did you get stuck anywhere?

No. This was more or less a smooth week...

Marking the end of Phase 1

GSoC page says that the first phase reviews would start from tomorrow. So, this seems like a good time to summarize all the progress of Phase 1 here:

PR filed: Tests pass.
SciPy builds with UNU.RAN.
Separated UNU.RAN in its own submodule.
Create wrappers for one continuous and one discrete generator.
Basic benchmarks written.
Basic tests written.
A strong documentation suite and tutorials written.
Extra benchmarks written on my fork for all continuous methods in UNU.RAN.

According to my proposal, I have successfully achieved my first and second milestone and encroached the third milestone ahead of time! Most of the points above are still under reviews and might be changed in the future if need be. It would still be a challenge, both for the maintainers and me, to resolve the memory leaks issue but I hope that is done before the end of the second phase so that we can merge and test out some of the new functionality and iterate on the design. Let's hope the best for what's coming!

Week #4: Memory Leaks :(

tirthasheshpatel@gmail.com (tirthasheshpatel) — Sun, 04 Jul 2021 19:31:42 +0000

What did you do this week?

This week was, by far, the most challenging week for me! Turns out, resolving the memory leaks issue is not as simple as patching UNU.RAN and changing the order of frees and calls to the error handler :/ I was able to discover this because of a very helpful suggestion by Nicholas (one of my mentors) to run valgrind on my changes (see the discussion on tirthasheshpatel/unuran#1).

==25301== LEAK SUMMARY:
==25301==    definitely lost: 13,728 bytes in 58 blocks
==25301==    indirectly lost: 43,520 bytes in 387 blocks
==25301==      possibly lost: 166,439 bytes in 114 blocks
==25301==    still reachable: 3,553,979 bytes in 2,254 blocks
==25301==         suppressed: 0 bytes in 0 blocks

These memory leaks occur due to the non-local returns in the thunk and in the error handler. Unfortunately, this behavior is ingrained in the way I have designed the API to handle errors and a major refactor is required to fix the issue! tirthasheshpatel/scipy#9 is my first (and hopefully last) attempt aimed at doing this. It refactors the Cython wrapper to use the MessageStream API to handle the errors occurring in UNU.RAN and PyErr_Occurred() to detect the errors occurring in Python callbacks.

The MessageStream API was introduced and written by Pauli Virtanen (@pv on GitHub) while writing wrappers for Qhull. MessageStream uses FILE * streams to log errors occurring in the C API to a temporary file (either in memory or on disk depending on whether open_memstream is available). Once the execution of the C function is complete, one can check the file for errors and raise them in Python. One of the downsides of this approach is that UNU.RAN contains a global FILE * variable which is not thread-safe. Hence, thread-safety needs to be provided in the Cython wrapper itself which further complicates things. I have used a module-level lock which is acquired every time before calling the unur_set_stream function (which is responsible for changing the global FILE * variable) and is released once the required set of C functions have been evaluated. Finally, valgrind seems to be happy with this and reports no memory leaks!

==44175== LEAK SUMMARY:
==44175==    definitely lost: 1,128 bytes in 11 blocks
==44175==    indirectly lost: 0 bytes in 0 blocks
==44175==      possibly lost: 195,264 bytes in 124 blocks
==44175==    still reachable: 3,258,070 bytes in 2,112 blocks
==44175==         suppressed: 0 bytes in 0 blocks

(The 1128 lost bytes are inside dlopen.c library and not UNU.RAN wrappers.)

A very similar thing has been done by Pauli Virtanen in Qhull and getting his help and reviews on tirthasheshpatel/scipy#9 would be invaluable! I hope that my approach is correct this time and the whole fuss about memory leaks resolves as soon as possible.

What is coming up next?

The issue I delineate in the previous section is the bottleneck blocking #14215 from receiving more reviews and merging. I hope to continue my work on tirthasheshpatel/scipy#9 and get it approved and/or merged by the end of this or next week. I also hope this doesn't bring more surprises down the line and I can continue my work on #14215 more smoothly!

Did you get stuck anywhere?

Not yet... :)

Week #3: Separating UNU.RAN in its own submodule

tirthasheshpatel@gmail.com (tirthasheshpatel) — Sun, 27 Jun 2021 05:11:40 +0000

What did you do this week?

This week went by addressing last week's blockers and bifurcating UNU.RAN in its own submodule. I put the UNU.RAN source code in my PR which made that patch size over 100,000 LOC making it very difficult to review. So, I spent a couple of days creating a separate repository for the UNU.RAN source code and using git submodule to clone it into SciPy. I also wrote a Python script to download and clean UNU.RAN for use in SciPy. This reduced the size of the patch from over 100,000 LOC to only about 4000 LOC. There were also some comments on the tutorial and benchmarks which I addressed this week.

What is coming up next?

Last week, I also noticed a memory leak so I decided to write a Python script to find more. The script reports 40 to 50 memory leaks throughout the source code but the part of the source code used by SciPy has only about 9 to 10 memory leaks. In the last meeting, we came up with two potential ways to get rid of them: (i) patch up UNU.RAN and reverse the order of frees and error, (ii) Instead of jumping out of the C function, use the return code of UNU.RAN functions to set and raise the error. The problem with approach (ii) is that some functions don't return an error code. So, I plan to test the first approach out this week and hopefully address all the memory leaks. We also decided to try out writing a higher-level API for the String API and get other devs opinions on it on the mailing list.

Did you get stuck anywhere?

No blockers this week!

Week #2: Working on the PR

tirthasheshpatel@gmail.com (tirthasheshpatel) — Sun, 20 Jun 2021 11:05:08 +0000

What did you do this week?

This week was spent mostly polishing my pull request and addressing reviews. I got a few big things out of the way though. Firstly, I refactored the API to accept a single dist object containing all the required methods. Secondly, I wrote a tutorial to document the usage of the new API. And lastly, I wrote a benchmark suite to profile the setup and sampling stage of each sampler. Moreover, using a lot of help from Bas (@BvB93 on GitHub), I was able to resolve all the MyPy static typing errors and get the MyPy check passing. While adding tests for the seed parameter, I noticed that I had made a mistake in handling the old NumPy RandomState API. As I had used global variables to sample from the NumPy RNG, seeding a generator broke! This was because the underlying (global) NumPy RNG was overridden by a new RNG as soon as a new generator with a seed was created. Thankfully, I quickly found a way to avoid the use of global variables and tests started passing again. One of my mentors, Christoph, was interested in using the UNU.RAN's test suite to write strong tests in SciPy. I have started looking into its test suite and also ported a few tests but this is still a work in progress.

What is coming up next?

I have got a lot of work done on the PR and it's shaping nicely: Main components of the PR have been written; Most tests pass. With this, I hope to mark the PR as open for reviews soon. I will have to make sure that I have added sufficient tests and documentation. Also, the new code lacks comments which may give reviewers a difficult time. I aim to clean out the newly added code and write more comments to delineate certain parts that might be tricky to understand. I also need to clean up the license file. There was also interest in separating UNU.RAN in a submodule. I hope to address some of these points in the upcoming week.

Did you get stuck anywhere?

I faced a weird 32-bit Linux failure which was related to my changes. When the randint distribution is input to the DAU method, it fails with an "unknown error" in UNU.RAN. I was able to localize the error but failed to find a reason for the failure. I suspect floating-point errors but a deeper inspection needs to be done. For the time being, as this isn't inside SciPy (and also only exists on a very specific platform and an old NumPy version), I have skipped that test case. This also led to a squalid revelation: memory leaks :/ This is turning into more of a can of worms than I had initially expected. Sometimes UNU.RAN frees allocated memory after calling the error handler. But the error handler is designed to jump out of the C code and return to the Cython code where the error can be raised safely. But, then, the allocated memory is never freed leading to a memory leak. I am not sure how often this happens. But it might be something to investigate in more depth. I will see if this is substantial and look into what can be done.

Week #1: Creating a (big) PR

tirthasheshpatel@gmail.com (tirthasheshpatel) — Sat, 12 Jun 2021 19:58:20 +0000

What did you do this week?

This week I submitted an overview of the progress on the mailing list (here) and created a pull request on SciPy (#14215). Thankfully, all the tests pass and SciPy builds with UNU.RAN on all the required platforms! I also created some flowcharts to elucidate the design of the internal API and manifest how callbacks are acquired and released. I also tried to write a higher-level API (tirthasheshpatel/scipy#8) as suggested by one of my mentors.

What is coming up next?

We have discussed quite a lot of points to keep me busy for a couple of weeks down the line :). Here it is:

Generate/build UNU.RAN tests and try integrating into SciPy test suite.
Maybe figure out a way to speed up the performance on NumPy < 1.19.
Write better/stronger tests.
Mock up API that uses same object interface i.e. bundle all functions together in a dist parameter.
Address code reviews on my PR.
Add relation [of the UNU.RAN API] to the rv_discrete and rv_continuous classes in tutorial. Add in docs that rvs of UNU.RAN methods and SciPy distributions differ.

Did you get stuck anywhere?

No blockers this week!

Week #0: Polishing the Prototype

tirthasheshpatel@gmail.com (tirthasheshpatel) — Mon, 07 Jun 2021 21:06:36 +0000

A little introduction

Hello everyone! I am Tirth, a last year computer science undergraduate student at Nirma University in India. I have been using NumPy and SciPy since I started doing scientific computing in my first year of college. I have been contributing to SciPy since last year and hope to continue to do so :). I will we working this summer to integrate UNU.RAN library in the scipy.stats submodule. UNU.RAN is a C library for Universal Non-Uniform RANdom number generation. It has been used in the ROOT project by CERN and R bindings for the library (Runuran) have also been created. My goal would be to integrate methods for sampling from univariate continuous and discrete distributions.

What did you do this week?

I got to know my mentors, Christoph and Nicholas, in the first week of the community bonding period. Since then, we have been meeting regularly to discuss the API and have been exchanging a lot of design ideas. Over the span of last three weeks, I have been able to significantly enhance my prototype to the point where I feel confident enough to propose a PR on SciPy. I started out with tirthasheshpatel/scipy#5 on my fork which was thread-unsafe and made my way up to tirthasheshpatel/scipy#6 which seems in a very good shape. It builds with UNU.RAN on all the required platforms and tests pass with an exception of a flaky failure. During the last week, I have created this excel sheet with some information on the methods I propose to add in SciPy. It also contains the information about the methods to add and parameters to keep, etc. It will help me with coding those methods in the coming weeks and also document the decisions properly.

What is coming up next?

As the PR on my fork builds and tests pass, I hope to create a PR on SciPy by next week. I also aim to circulate a mail in the Mailing List regarding the imminent PR and try to get feedback from other devs on the design of the API. Nevertheless, the coming weeks are critical to the work I aim to finish during GSoC so I hope to get things done without much contention!

Did you get stuck anywhere?

There have not been any serious blockers during the community bonding period but the Windows CI failed due to some unrelated Pythran errors. After a few abortive attempts to resolve them, the discussion on #13717 helped me fix the failing builds. As pointed out in this comment, I was missing LLVM and MinGW binaries in the PATH which caused some weird linking problems for 64-bit builds. It was moment of relief to see builds passing on windows, since windows failures worried me the most. Hopefully, everything passes on the SciPy PR that I aim to create by the end of this week :). Fingers crossed!