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

Unique() inconsistencies #621

Open
ClaudiaComito opened this issue Jul 7, 2020 · 2 comments · May be fixed by #749
Open

Unique() inconsistencies #621

ClaudiaComito opened this issue Jul 7, 2020 · 2 comments · May be fixed by #749
Assignees
Labels
bug Something isn't working manipulations

Comments

@ClaudiaComito
Copy link
Contributor

Description
While looking into #564, I found a number of inconsistencies in ht.unique().

  • Add clarification to documentation of unique() #564 is no documentation issue.

    • The kwarg sorted is set to False by default for ht.unique(), but it's True by default for torch.unique(). Currently, ht.unique(a) results in a sorted DNDarray if a.split=None (pure torch implementation: ht.unique(a) = torch.unique(a._DNDarray__array)), whereas if a.split is not None, the result will not be sorted (heat implementation).
    • Replacing ht.unique(a) = torch.unique(a._DNDarray__array) with
      ht.unique(a) = torch.unique(a._DNDarray__array, sorted=sorted) doesn't help, because sorted=False means different things for heat and torch:
      • heat interpretation: leave result unsorted;
      • torch interpretation: leave result REVERSE SORTED. See discussion on Add clarification to documentation of unique() #564 for an example.
        I propose setting sorted=True by default in ht.unique() as at the moment it's the only way to prevent inconsistencies with torch, although I'm aware that the sorting comes with significant overhead. Incidentally, numpy.unique() returns the "sorted unique elements of an array" and not sorting is not even an option.
  • if return_inverse=True, ht.unique() by design returns a list of one DNDarray (the unique elements) and one torch tensor (the inverse indices). Should be two DNDarrays.

  • it is currently not possible to run ht.unique(a, sorted=True, axis=axis) if axis != split. Error message:

    Sorting with axis != split is not supported yet. See vectorized sorting #363

    This needs to be followed up.

To Reproduce
Steps to reproduce the behavior:

  1. Which module/class/function is affected?
    manipulations.unique()

  2. What are the circumstances under which the bug appears?
    see above

  3. What is the exact error message / erroneous behavior?
    see above

Version Info
current main branch

@ClaudiaComito ClaudiaComito added the bug Something isn't working label Jul 7, 2020
@ClaudiaComito ClaudiaComito self-assigned this Jul 7, 2020
@ClaudiaComito ClaudiaComito linked a pull request Mar 24, 2021 that will close this issue
4 tasks
@ClaudiaComito ClaudiaComito linked a pull request Mar 30, 2021 that will close this issue
4 tasks
@ClaudiaComito ClaudiaComito added this to the Repo Clean-Up milestone Jul 31, 2023
@ClaudiaComito
Copy link
Contributor Author

Still open and will be fixed with #749


Reviewed within #1109

@github-actions
Copy link
Contributor

github-actions bot commented Oct 9, 2023

Branch bugs/621-Unique_inconsistencies created!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working manipulations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant