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

MAINT: Update to UNU.RAN 1.9.0 #2

Merged
merged 2 commits into from
Apr 28, 2022
Merged

MAINT: Update to UNU.RAN 1.9.0 #2

merged 2 commits into from
Apr 28, 2022

Conversation

tirthasheshpatel
Copy link
Member

@tirthasheshpatel tirthasheshpatel commented Mar 17, 2022

UNU.RAN 1.9.0 was released yesterday. This release contains a few bug fixes. In particular:

  • This resolves the segmentation faults in NumericalInversePolynomial caused by uniform densities.
  • Removes some legacy macros
  • Replaces the use of INFINITY with UNUR_INFINITY (but we have updated to C99 so this should not change anything)

Note to reviewers: I will propose a pull request on SciPy main cloning this branch to check if it builds on all the systems and resolves the segfault. Once that's working we can merge this and then merge the PR on SciPy main.

The SciPy PR is up: scipy/scipy#15798

Copy link
Member

@chrisb83 chrisb83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of changed files... it is mainly due to the change from INFIITY to UNUR_INFINITY.

As expected, some more changes are in the pinv files, though I don't know how the issue with constant densities is fixed. Tirth's PR in the scipy main repo shows that the related test now passes

Other changes are the addition of two new distributions (meixner and variance gamma) and some enhancements related to special functions.

There are references to GPL that we need to discuss.

unuran/src/distributions/c_beta_gen.c Show resolved Hide resolved
unuran/src/distributions/c_meixner.c Show resolved Hide resolved
unuran/src/distributions/c_vg.c Show resolved Hide resolved
unuran/src/specfunct/cgamma.c Show resolved Hide resolved
*
* Copyright (C) 1996, 1997, 1998, 1999, 2000, 2007 Gerard Jungman, Brian Gough, Patrick Alken
*
* This program is free software; you can redistribute it and/or modify
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again GPL is mentioned

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hypot is a C99 function. So, we can define HAVE_DECL_HYPOT and remove this file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC hypot is still special-cased by NumPy on Windows: https://github.com/numpy/numpy/blob/43c113dd7aa36ef833315035858ea98a7b4732c5/numpy/core/src/common/npy_config.h#L73. I'm not sure if that's still relevant or not, but may be worth double checking.

unuran/src/methods/unur_methods.h Show resolved Hide resolved
@chrisb83
Copy link
Member

Additional comment on the license topic:
We were given the permission to publish UNU.RAN 1.8.0 under the BSD license. We then reported a bug and the UNURAN maintainers told us that we can use the new release to fix it. So I do not see a problem if GPL is mentioned as long it is clear that the code is written by the maintainers or taken from someone else that allows to distribute it freely (like in spectfunc/cgamma.c).

Only spectfunc/hypot.c might be a problem since it uses code that someone else published under GPL

@chrisb83
Copy link
Member

Even for hypot.c, it looks like Joseph added the comment about the GPL licence and it is not clear if the authors mentioned imposed any licence restrictions. It is a very simple code to compute sqrt(x**2 + y**2) in a way that is numerically stable.

Copied and renamed from gsl_hypot into _unur_hypot
 * by Josef Leydold, Tue Nov  1 11:17:37 CET 2011
 * 
 * Copyright (C) 1996, 1997, 1998, 1999, 2000, 2007 Gerard Jungman, Brian Gough, Patrick Alken

@tirthasheshpatel
Copy link
Member Author

though I don't know how the issue with constant densities is fixed

In line 396 of ./unuran/src/methods/pinv_newton.h (function _unur_pinv_newton_create), the derivative of the PDF was called if the density didn't change. But dPDF hasn't been listed as a requirement for pinv. Which is why the code segfaulted. This has been fixed by filling in infinity. Here's the diff:

image

There are references to GPL that we need to discuss.

Thanks for noticing that! I will look into the GPLed code.

@tupui
Copy link
Member

tupui commented Mar 21, 2022

Additional comment on the license topic: We were given the permission to publish UNU.RAN 1.8.0 under the BSD license. We then reported a bug and the UNURAN maintainers told us that we can use the new release to fix it. So I do not see a problem if GPL is mentioned as long it is clear that the code is written by the maintainers or taken from someone else that allows to distribute it freely (like in spectfunc/cgamma.c).

Only spectfunc/hypot.c might be a problem since it uses code that someone else published under GPL

I would then update both Readme and license.txt to have a clear history of this. I would still remove the mention of GPL in the files because it could confuse automatic tools looking into licenses. It would be bad if it flagged this repo as GPL.

@tirthasheshpatel
Copy link
Member Author

There are references to GPL that we need to discuss.

I looked into this. We don't use any of the GPLed code other than the one written by the authors (we should be safe to use that and remove those references to GPL)

@chrisb83
Copy link
Member

We don't use any of the GPLed code other than the one written by the authors (we should be safe to use that and remove those references to GPL)

I agree.

Do you have an idea on how to proceed with hypot? I did not understand if the NumPy issue numpy/numpy#9567 is relevant in our situation

@tirthasheshpatel
Copy link
Member Author

Do you have an idea on how to proceed with hypot?

I think numpy/numpy#9567 is still relevant. To avoid using the GPL code, I just copied NumPy's hypot and switch to it for 32-bit windows (which causes problems for inf input). As discussed, we have permission to use other code. I will update the SciPy PR to check if the latest changes work.

@chrisb83
Copy link
Member

Good news: It looks like the failures in the SciPy PR are unrelated to your changes (timeouts or the known failure of truncnorm)

(It looks funny that we replace the hypot code with the same code from np (modulo variable names :))

@tirthasheshpatel
Copy link
Member Author

It looks funny that we replace the hypot code with the same code from np (modulo variable names

Yeah, I have heard even simple GPLed code can gen one into a lot of trouble. Now, no one can blame us for using GPL code :)

Copy link
Member

@chrisb83 chrisb83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The update looks good (I do not see a license issue. In particular, hypot in NumPy replaces the GPLed version).

@tirthasheshpatel
Copy link
Member Author

@chrisb83 This has been sitting around for a while now. Can you merge? I will update scipy/scipy#15798 and we can merge that one too.

@chrisb83
Copy link
Member

Sure, I did not know that I am allowed to merge in this project. Since there are no open points, I will do it right away.

@chrisb83 chrisb83 merged commit c7c1e5c into main Apr 28, 2022
@tirthasheshpatel
Copy link
Member Author

Thanks @chrisb83!

@tirthasheshpatel tirthasheshpatel deleted the unuran_1_9_0 branch April 28, 2022 17:56
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.

4 participants