Overflow condition and negative argument for 'ibin'

[Arpan612]

Overflow condition and negative argument for 'ibin'

References to other Issues or PRs

Fixes #18963

Brief description of what is fixed or changed

Currently, sympy.utilitities.iterables.ibin passes silently, returning all bits of a number even if the requested bit length is too small. The user can omit the bits argument to get all bits; they should be warned if they request a certain length and the number has more than that many bits since this could indicate a logical error in their code. e.g. 3 bits are required but the user-specified 2 bits
Also, a negative argument is given error condition

Other comments

Release Notes

• utilities
  • Overflow condition and negative argument for 'ibin' added

[sympy-bot]

✅

Hi, I am the SymPy bot (v158). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

• utilities
  • Overflow condition and negative argument for 'ibin' added (#18973 by @Arpan612)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.6.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->
Overflow condition and negative argument for 'ibin'
#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->
Fixes #18963 

#### Brief description of what is fixed or changed
Currently, sympy.utilitities.iterables.ibin passes silently, returning all bits of a number even if the requested bit length is too small. The user can omit the bits argument to get all bits; they should be warned if they request a certain length and the number has more than that many bits since this could indicate a logical error in their code. e.g. 3 bits are required but the user-specified 2 bits
Also, a negative argument is given error condition
#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* utilities
  * Overflow condition and negative argument for 'ibin' added
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

[Arpan612]

@smichr Could you please suggest some changes? My doctest is failing continuously.

[Arpan612]

@smichr Got it sir. Working on it.

[Arpan612]

@smichr Could you please go through the changes? Travis is not working.

[Arpan612]

Closed by mistake. Sorry.

[Arpan612]

@smichr Work here is almost done. I am getting an error for using n.bitlength(). Could you help me out?

[smichr]

OK, now 2 tests would be good: ibin(-.5) and ibin(2,1). The first one makes sure that int is not applied to the arg before testing and that a negative argument will raise and the second one tests the check that there are enough bits to represent the number.

[Arpan612]

Yes sir. On it.

[Arpan612]

@smichr Sir, with due respect, the Update iterables.py you committed was throwing a travis error so I corrected it. I hope it is fine with you. Could you please correct the ibin(-0.5)? I think that is the last work left in this PR.

[Arpan612]

@smichr Sir, please help me with the ibin(-0.5) condition.

[smichr]

was throwing a travis error

I see: the bits='all' sort of condition is caught in the try.

Note how raises is used to test statements - see last changes.

[Arpan612]

@smichr Sir, the error persists. Is it fine if we remove the -0.5 test and go back to the 2 test which was working fine?

[codecov]

Codecov Report

Merging #18973 into master will decrease coverage by 0.025%.
The diff coverage is 100.000%.

@@              Coverage Diff              @@
##            master    #18973       +/-   ##
=============================================
- Coverage   75.784%   75.759%   -0.026%     
=============================================
  Files          647       648        +1     
  Lines       168657    168803      +146     
  Branches     39745     39791       +46     
=============================================
+ Hits        127816    127884       +68     
- Misses       35279     35353       +74     
- Partials      5562      5566        +4     

[smichr]

which was working fine?

getting an overflow answer when you are trying not to do so is...not working ;-)

It should be ready to go now.

[smichr]

It would be good to clean up the commit history:

1. squash your commits into a single commit
2. cherry-pick the last commit from my ibin branch (rebase on master if necessary)
3. force push the result here

[Arpan612]

@smichr I am trying to do this but since your commits are there as well, my terminal is constantly throwing an error. Moreover, the commits help the reviewer walk through the changes made. Is it okay if we merge this PR as it is?

[Arpan612]

@smichr Sir, I tried again but the issue persists. Please can we merge this PR?

[smichr]

my terminal is constantly throwing an error

go ahead and post the errors

[smichr]

Any easy way to make a single commit from the current state is the following:

git merge master
git reset head~2 --hard
git diff master > dif
git checkout master
git branch -D issue.18963
git checkout -b issue.18963
git apply dif

then cherrypick the last commit from my ibin

[Arpan612]

fatal: ambiguous argument 'head~2': unknown revision or path not in the working tree.
I am getting this error

[Arpan612]

Let me try to manually do the changes in my file and then push

[jksuom]

fatal: ambiguous argument 'head~2': unknown revision or path not in the working tree

That should probably be 'HEAD~2' if your system is sensitive to the case of characters in file names.

[Arpan612]

@jksuom @smichr I have done the needful.