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

Integrate part of scipy elliptic functions and integrals #3111

Merged
merged 6 commits into from
Jun 5, 2022

Conversation

RandomY-2
Copy link
Contributor

@RandomY-2 RandomY-2 commented Jun 2, 2022

What do these changes do?

Integrate scipy elliptic functions and integrals except ellipj which returns multiple outputs. Some functions are within a try except block because they were not implemented in scipy v1.7, so spspecial.func may throw an AttributeError if the scipy version is low.

Related issue number

#751 (except ellipj)

Check code requirements

  • tests added / passed (if needed)
  • Ensure all linting tests pass, see here for how to run them

* elliptic functions and integrals init

* Update special.rst

* Update ellip_func_integrals.py

* ellipk

* ellipk test

* elliptic functions and integrals except ellipj

* skip tests if scipy version is too low

* test getattr import

* try for new scipy functions

* comment for try catch

* change to getattr

* Update ellip_func_integrals.py

* test benchmark

* Update ellip_func_integrals.py

* change exception handling

* update pytest reason

* black reformat

* Update ellip_func_integrals.py

* change exception handling

* Update test_special_execution.py
@RandomY-2 RandomY-2 requested a review from a team as a code owner June 2, 2022 18:23


@pytest.mark.skipif(
scipy.__version__ < "1.8.0", reason="function not implemented in scipy."
Copy link
Member

Choose a reason for hiding this comment

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

Comparing versions by strings is not safe. Try looking up parse_version in the code repo and see how we handle dependency versions like pandas.

@@ -12,7 +12,10 @@
# See the License for the specific language governing permissions and
# limitations under the License.

from ....lib.version import parse as parse_version
Copy link
Member

Choose a reason for hiding this comment

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

We used to put imports in builtin-thirdparty-Mars order. Please take a look at other py files and change the order.

Copy link
Member

@wjsi wjsi left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution!

@wjsi wjsi added mod: tensor type: feature New feature to be backported Indicate that the PR need to be backported to stable branch labels Jun 4, 2022
@wjsi wjsi added this to the v0.10.0a1 milestone Jun 4, 2022
Copy link
Collaborator

@qinxuye qinxuye left a comment

Choose a reason for hiding this comment

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

LGTM

@qinxuye qinxuye merged commit 24290fe into mars-project:master Jun 5, 2022
@qinxuye qinxuye added backported already PR has been backported and removed to be backported Indicate that the PR need to be backported to stable branch labels Jun 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported already PR has been backported mod: tensor type: feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants