Skip to content

Conversation

@MasonProtter
Copy link
Contributor

No description provided.

@MasonProtter
Copy link
Contributor Author

The values I'm comparing against in the tests come from Mathematica's SphericalBessejJ and SphericalBesselY by the way.

Co-Authored-By: Alex Arslan <ararslan@comcast.net>
@ararslan ararslan dismissed their stale review January 8, 2020 23:28

I don't know enough about spherical Bessel functions to feel comfortable merging but this looks reasonable to me if Steven agrees.

@ViralBShah
Copy link
Member

It may be better to not add new functionality to this package and instead build newer packages. This package is largely a wrapper around fortran code.

@ararslan
Copy link
Member

ararslan commented Mar 9, 2020

I disagree, I think this is a fine home for special functions in general. They can be replaced with pure Julia implementations piecemeal and perhaps sometime in the future they'll all be rewritten and we can just drop the binary dependency from this package without a disruption to users.

@giordano
Copy link
Member

giordano commented Mar 9, 2020

I agree with Alex. OpenSpecFun can still be useful for testing, but I don't see the need to move elsewhere

@musm
Copy link
Contributor

musm commented Mar 10, 2020

I also agree with Alex. This package isn't really a thin wrapper around fortran or openspecfun. I also see no tension between holding both openspec and fortran wrappers in the same package. In the future we can always rewrite functions in pure Julia, given time and effort. It seems too fragmented to split off things into a spererat package.

@musm
Copy link
Contributor

musm commented Mar 11, 2020

This PR LGTM, needs a rebase

@musm musm requested a review from dlfivefifty April 1, 2020 14:09
@dlfivefifty
Copy link
Member

If someone can resolve the conflict (which I don't quite understand) and tag a new release I think this can be merged.

Since this makes new exports I guess it should technically be a minor tag?

Copy link
Contributor

@musm musm left a comment

Choose a reason for hiding this comment

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

this is the patch, not the minor version

@MasonProtter
Copy link
Contributor Author

CI failure appears to be unrelated.

@musm
Copy link
Contributor

musm commented Apr 5, 2020

LGTM not 100% on the threshold level, but we can revisit or look into it later in more detail

@musm musm merged commit 2820bea into JuliaMath:master Apr 5, 2020
solution to the radial part of the Helmholz equation in spherical coordinates. Sometimes
known as a spherical Neumann function.
"""
sphericalbessely(nu, x::T) where {T} = ((float(T))(π)/2x) * bessely(nu + 1//2, x)
Copy link
Contributor

@musm musm Apr 5, 2020

Choose a reason for hiding this comment

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

Shouldn't this also be just

nu + 1/2

instead of

1+1//2

cc @MasonProtter ?

Copy link
Member

Choose a reason for hiding this comment

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

Think you mean one(T)/2

Copy link
Contributor

@musm musm Apr 5, 2020

Choose a reason for hiding this comment

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

one(nu) actually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, my bad.

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.

8 participants