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

Adding jax.numpy.histogram #21188

Closed
wants to merge 0 commits into from
Closed

Adding jax.numpy.histogram #21188

wants to merge 0 commits into from

Conversation

marwanWaly
Copy link

@marwanWaly marwanWaly commented Aug 2, 2023

I wrote everything as it should be according to docs and other functions in the same module but I have some problems:

  1. I cannot run other unit tests in the same module, although I was able to run them before my last pull.
    when I try I face this error:
    `ImportError while loading conftest '/opt/project/ivy_tests/test_ivy/conftest.py'.
    ../../../conftest.py:7: in
    import ivy_tests.test_ivy.helpers.test_parameter_flags as pf
    ../../../helpers/init.py:2: in
    from . import hypothesis_helpers
    ../../../helpers/hypothesis_helpers/init.py:1: in
    from . import general_helpers
    ../../../helpers/hypothesis_helpers/general_helpers.py:8: in
    import ivy
    /opt/project/ivy/init.py:770: in
    from . import stateful
    /opt/project/ivy/stateful/init.py:1: in
    from . import activations
    /opt/project/ivy/stateful/activations.py:5: in
    from ivy.stateful.module import Module
    /opt/project/ivy/stateful/module.py:8: in
    import dill
    E ModuleNotFoundError: No module named 'dill'

Process finished with exit code 4

Empty suite`

  1. I used ivy.histogram but while checking the implementation in jax backend, I found it in
    ivy/functional/backends/jax/experimental/statistical.py
    and didn't find it in
    ivy/functional/backends/jax/statistical.py,
    so I am not sure is that normal or not?

  2. Same for _histogram_helper(), I used this function in while creating the unit test but I have imported it like this:
    from ivy_tests.test_ivy.test_functional.test_experimental.test_core.test_statistical import ( _histogram_helper, )
    and coudn't find it in: ivy_tests.test_ivy.test_functional.test_experimental.test_core.test_statistical

@ivy-leaves ivy-leaves added the JAX Frontend Developing the JAX Frontend, checklist triggered by commenting add_frontend_checklist label Aug 2, 2023
@ivy-leaves
Copy link

If you are working on an open task, please edit the PR description to link to the issue you've created.

For more information, please check ToDo List Issues Guide.

Thank you 🤗

@marwanWaly
Copy link
Author

Sorry I forget it.
Close #19652

Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove the code which specifies the arg types. (Eg:- Union[ivy.Array, ivy.NativeArray] etc for a). You can refer to other implementations in this file for reference too. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

Please also remove the merge conflicts @marwanWaly. Thanks!

@theRealBird
Copy link
Contributor

Hi @marwanWaly,
Thank you so much for your contribution to Ivy and for your patience with the PR review process. To answer your questions:-

  1. I am not too sure about the error with "dill". I was unable to replicate it at my end. I am not sure if you tried this but maybe try installing the dill package using pip in your environment? That seems to work for me usually.

  2. Yeah we shouldn't worry about the helpers being in experimental. It's just how we have structured the project.

At the moment, the CI dashboard is telling me that, there are a lot of new test failures being introduced. They don't seem to be related to your code but I would suggest you to have a look at them once more.

Regarding the actual implementation, everything looks great except maybe one thing which I have requested to be changed. Thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JAX Frontend Developing the JAX Frontend, checklist triggered by commenting add_frontend_checklist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants