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

accept Number in Exponential #1691

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

accept Number in Exponential #1691

wants to merge 2 commits into from

Conversation

rafaqz
Copy link

@rafaqz rafaqz commented Mar 8, 2023

This is a start on #1684, with Exponential as an example to start the discussion, I'm not personally across the problems this may cause so this PR is a request for input and feedback on that.

The idea is to loosen the type of θ to be Number, and add a check for Complex numbers so those errors are avoided. More or different checks may be required.

I'm not totally satisfied with the combination of !(θ isa Complex) and isreal(θ) checks, but the idea is to make sure that even wrapped numbers are internally Real.

@codecov-commenter
Copy link

codecov-commenter commented Mar 8, 2023

Codecov Report

Patch coverage is 66.66% of modified lines.

Files Changed Coverage
src/univariate/continuous/exponential.jl 66.66%

📢 Thoughts on this report? Let us know!.

@ParadaCarleton
Copy link
Contributor

The problem here is related to Unitful, right? I'm happy to merge this if we need to, but first I'd like to check if it's possible to fix this in Unitful. @sostock ?

@devmotion
Copy link
Member

IMO this needs more thought as it affects much more than just Exponential (see #1684). I also agree that the checks introduced in this PR are suboptimal.

@rafaqz
Copy link
Author

rafaqz commented Aug 23, 2023

Its not related to Unitful.jl - its a generic problem with Number.

See here:
rafaqz/ModelParameters.jl#51

I don't have bandwidth (or the required knowledge) to make this "optimal" or think through the implications, that is clearly a problem for devs of this package.

I was just proposing a solution that would work for other users of my own packages, as well as Unitful.jl

@sostock
Copy link

sostock commented Sep 2, 2023

Another option would be to make the support for non-Real types opt-in, so that this package only supports Exponential{<:Real}, but other packages can add support for their types as package extensions.

For that, one would change the type parameter to T<:Number and rename the internal constructor to something like unsafe_exponential while providing an outer constructor only for Exponential{<:Real}. Other packages (like Unitful.jl) can then create a package extension in which they define their own outer constructor that calls the inner unsafe_exponential function.

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>
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.

5 participants