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

Add Range and Array modeling types #2510

Merged
merged 54 commits into from Jun 22, 2021
Merged

Add Range and Array modeling types #2510

merged 54 commits into from Jun 22, 2021

Conversation

ktbarrett
Copy link
Member

@ktbarrett ktbarrett commented Apr 3, 2021

Follow on to #2485. xref #2059. Adds a Range and Array type.

The Range type is much like Python's range type, but has an inclusive right bound like is seen in both Verilog and VHDL. It support's VHDL's to and downto directions, Verilog's automatic directionality by not specifying a direction, and VHDL's null ranges.

The Array type is a heterogenous fixed-length sequence type. It uses an initial value or a range to determine it's size at construction and it's size is fixed. It supports must of the same operations as a list that don't mutate the length. Uses a Range to describe the indexing scheme, which is arbitrary, like is seen in both Verilog and VHDL. Some indexing and slicing behavior had to change compared to a list to make something that behaved intuitively.

@ktbarrett ktbarrett added the type:feature new or enhanced functionality label Apr 3, 2021
@ktbarrett ktbarrett added this to the 2.0 milestone Apr 3, 2021
@ktbarrett ktbarrett requested a review from a team April 3, 2021 06:47
cocotb/types/array.py Outdated Show resolved Hide resolved
cocotb/types/array.py Outdated Show resolved Hide resolved
cocotb/types/array.py Outdated Show resolved Hide resolved
cocotb/types/array.py Outdated Show resolved Hide resolved
cocotb/types/array.py Outdated Show resolved Hide resolved
cocotb/types/range.py Outdated Show resolved Hide resolved
cocotb/types/range.py Outdated Show resolved Hide resolved
cocotb/types/range.py Outdated Show resolved Hide resolved
cocotb/types/range.py Outdated Show resolved Hide resolved
cocotb/types/range.py Outdated Show resolved Hide resolved
@ktbarrett
Copy link
Member Author

ktbarrett commented Apr 3, 2021

I keep forgetting that cache and cached_property aren't available until 3.8. For the current use of cached_property, I could just alias property and those using less than 3.8 will suffer from slightly lower performance? Or I can just switch to using both the lru_cache and property decorators together.

cocotb/types/range.py Outdated Show resolved Hide resolved
cocotb/types/range.py Outdated Show resolved Hide resolved
cocotb/types/range.py Outdated Show resolved Hide resolved
cocotb/types/array.py Outdated Show resolved Hide resolved
cocotb/types/range.py Outdated Show resolved Hide resolved
cocotb/types/array.py Show resolved Hide resolved
cocotb/types/range.py Outdated Show resolved Hide resolved
tests/pytest/test_array.py Outdated Show resolved Hide resolved
tests/pytest/test_range.py Show resolved Hide resolved
cocotb/types/array.py Outdated Show resolved Hide resolved
@ktbarrett
Copy link
Member Author

ktbarrett commented Apr 3, 2021

Using lru_cache and property together to mock cached_property only works if the object is hashable, which Arrays are not since they are mutable. So I went back to the original idea of just aliasing property to cached_property if it isn't available. For now people will have to deal with the performance drop (it shouldn't be noticeable anyways).

@codecov
Copy link

codecov bot commented Apr 3, 2021

Codecov Report

Merging #2510 (7a18d2c) into master (c065ccb) will increase coverage by 0.81%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2510      +/-   ##
==========================================
+ Coverage   74.46%   75.28%   +0.81%     
==========================================
  Files          38       40       +2     
  Lines        6537     6752     +215     
  Branches      560      596      +36     
==========================================
+ Hits         4868     5083     +215     
  Misses       1485     1485              
  Partials      184      184              
Impacted Files Coverage Δ
cocotb/types/__init__.py 100.00% <100.00%> (ø)
cocotb/types/array.py 100.00% <100.00%> (ø)
cocotb/types/range.py 100.00% <100.00%> (ø)

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 c065ccb...7a18d2c. Read the comment docs.

@cmarqu
Copy link
Contributor

cmarqu commented Apr 3, 2021

We should also add cocotb/types/array.py and cocotb/types/range.py after

cocotb/binary.py
so that the doctests are also run.

@ktbarrett
Copy link
Member Author

@cmarqu if you think there are any other exception that could improve, please comment.

I need to refactor the __init__ on Array slightly so I can better check that range argument, if specified, is in fact a Range since I depend on it's implementation in the body.

@ktbarrett
Copy link
Member Author

Resolved issue where constructor would not check if range value was a Range. The new implementation allows easy extension of type/value checking, etc.

@ktbarrett
Copy link
Member Author

Not entirely sure if the last set of changes is welcome, but it should make extending Array for LogicArray simpler. I already wanted to encapsulate range construction, since it was done in more than one place and needed to do type checking. We could even extend _construct_range to handle an iterable that can be splatted into a Range constructor to be able to support Array(range=(1, 'to', 8)) without having to explicitly construct the value into a Range first, however, that might be of dubious merit.

cocotb/types/array.py Outdated Show resolved Hide resolved
cocotb/types/array.py Outdated Show resolved Hide resolved
cocotb/types/array.py Outdated Show resolved Hide resolved
cocotb/types/array.py Outdated Show resolved Hide resolved
cocotb/types/array.py Outdated Show resolved Hide resolved
cocotb/types/range.py Outdated Show resolved Hide resolved
cocotb/types/range.py Outdated Show resolved Hide resolved
cocotb/_py_compat.py Outdated Show resolved Hide resolved
cocotb/types/array.py Outdated Show resolved Hide resolved
cocotb/types/array.py Outdated Show resolved Hide resolved
cocotb/types/array.py Outdated Show resolved Hide resolved
cocotb/types/range.py Outdated Show resolved Hide resolved
cocotb/types/array.py Outdated Show resolved Hide resolved
cocotb/types/array.py Outdated Show resolved Hide resolved
cocotb/types/range.py Show resolved Hide resolved
cocotb/types/array.py Show resolved Hide resolved
cocotb/types/array.py Show resolved Hide resolved
@ktbarrett
Copy link
Member Author

I think that's enough last minute cleanup @cocotb/maintainers. If there are no more comments in a couple days, I will merge.

@ktbarrett ktbarrett merged commit 8abe491 into cocotb:master Jun 22, 2021
@ktbarrett ktbarrett deleted the array branch June 22, 2021 04:08
cmarqu added a commit to cmarqu/cocotb that referenced this pull request Feb 27, 2022
* Add Range modeling type

Range datatype mimic's Python "range" type, but with inclusive right
bounds like is seen in HDLs.

* Add Array modeling type

* Apply suggestions from code review

Co-authored-by: Colin Marquardt <cmarqu42@gmail.com>

* Use compat layer for cache and cached_property

* Run black

* Apply suggestions from code review

Co-authored-by: Colin Marquardt <cmarqu42@gmail.com>

* Remove 'you' from docstrings

* Run pyupgrade --py36-plus

* fix compat

* Apply suggestions from code review

Co-authored-by: Colin Marquardt <cmarqu42@gmail.com>

* Fix compat again...

* Add doctests

* Improve indexing exception

* More detail in exceptions

* Improve constructor

* encapsulate construction of value and range for subclasses

* Apply suggestions from code review

Co-authored-by: Colin Marquardt <cmarqu42@gmail.com>

* Condense notes

* don't use cached_property anymore

* Improve equality

* Update cocotb/types/range.py

Co-authored-by: Eric Wieser <wieser.eric@gmail.com>

* Improve error message

* inline unnecessarily split function

* Fix a bug

* Explicitly state that right bound is inclusive

* Improve method documentation

* Apply suggestions from code review

Co-authored-by: Colin Marquardt <cmarqu42@gmail.com>

* Removed unused definition

* Improve equality behavior

* Improve documentation on equality and other operations

* Use double quotes in docstrings like black

* Improve warnings on differences with builtin sequence types

* Simplify array element construction

* Update cocotb/types/array.py

* 100% coverage baby...

* Apply suggestions from code review

* Update cocotb/types/range.py

Co-authored-by: Eric Wieser <wieser.eric@gmail.com>

* Remove .length

* Move 'concat' to base array

* Apply suggestions from code review

* Allow user to change bounds after array creation

* mypy fixes

* Additional black fixes

* Further take Eric's advice and simplify the array code more

* Fix mypy fixes

* Make concat a free binary operation

* Apply suggestions from code review

* Improve concat implementation

* Fixes for special method resolution

* Fix __concat__=None problem

* Fix occurences of __class__

* Final black run

* Add note about constructing ranges

* Add concat() to reference documentation

Co-authored-by: Colin Marquardt <cmarqu42@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature new or enhanced functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants