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

randint accept ints for 'size' #916

Merged
merged 4 commits into from
Feb 14, 2022
Merged

randint accept ints for 'size' #916

merged 4 commits into from
Feb 14, 2022

Conversation

mtar
Copy link
Collaborator

@mtar mtar commented Feb 8, 2022

Description

Issue/s resolved: #913

Changes proposed:

  • allow ints for size in randint

Type of change

  • Bug fix

Memory requirements

n/a

Performance

n/a

Due Diligence

  • All split configurations tested
  • Multiple dtypes tested in relevant functions
  • Documentation updated (if needed)
  • Updated changelog.md under the title "Pending Additions"

Does this change modify the behaviour of other functions? If so, which?

no

@mtar mtar added the bug Something isn't working label Feb 8, 2022
@codecov
Copy link

codecov bot commented Feb 8, 2022

Codecov Report

Merging #916 (2b617d4) into master (66da5c2) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #916   +/-   ##
=======================================
  Coverage   95.49%   95.49%           
=======================================
  Files          64       64           
  Lines        9793     9796    +3     
=======================================
+ Hits         9352     9355    +3     
  Misses        441      441           
Flag Coverage Δ
gpu 94.58% <100.00%> (+<0.01%) ⬆️
unit 91.07% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
heat/core/random.py 99.66% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66da5c2...2b617d4. Read the comment docs.

@mtar mtar changed the title fix size bug in randint randint accept ints as size Feb 8, 2022
@mtar mtar changed the title randint accept ints as size randint accept ints as 'size' Feb 8, 2022
@mtar mtar changed the title randint accept ints as 'size' randint accept ints for 'size' Feb 8, 2022
Copy link
Contributor

@ClaudiaComito ClaudiaComito left a comment

Choose a reason for hiding this comment

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

Thanks a lot @mtar for looking into this right away! I found another inconsistency vs. numpy, see comments.

shape = (int(size),)
else:
if not all(ele > 0 for ele in shape):
raise ValueError("negative dimensions are not allowed")

Copy link
Contributor

@ClaudiaComito ClaudiaComito Feb 9, 2022

Choose a reason for hiding this comment

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

I was thinking of addressing this like we do for shape in ht.reshape():

def reshape(a: DNDarray, *shape: Union[int, Tuple[int, ...]], **kwargs) -> DNDarray:
   ...

Default size should be 0, not None. Here a numpy example:

>>> d = np.random.randint(0,50)
>>> d
1
>>> d = np.random.randint(0,50, 1)
>>> d
array([0])
>>> e = ht.random.randint(0,50)
>>> e
DNDarray([9], dtype=ht.int32, device=cpu:0, split=None)  # ndim=1 but should be 0!

# size parameter allows int arguments
a = ht.random.randint(1, size=10, split=0, dtype=ht.int64)
self.assertTrue(ht.equal(a, b))

Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests where no size is specified and scalar is returned?

@mtar
Copy link
Collaborator Author

mtar commented Feb 14, 2022

run tests

Copy link
Contributor

@ClaudiaComito ClaudiaComito left a comment

Choose a reason for hiding this comment

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

Great, thanks @mtar !

@ClaudiaComito ClaudiaComito merged commit 0c89691 into master Feb 14, 2022
@ClaudiaComito ClaudiaComito deleted the bug/913-randint-size branch February 14, 2022 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ht.random.randint size type requirement inconsistent with NumPy API
2 participants