#140 #76 #30 #151 - Githubissues

codecov[bot] commented 1 year ago

Codecov Report

Merging #148 (1300968) into main (1e11ad8) will increase coverage by 0.39%. The diff coverage is 83.82%.

@@            Coverage Diff             @@
##             main     #148      +/-   ##
==========================================
+ Coverage   87.67%   88.06%   +0.39%     
==========================================
  Files          95       96       +1     
  Lines        8582     8633      +51     
==========================================
+ Hits         7524     7603      +79     
+ Misses       1058     1030      -28

Impacted Files	Coverage Δ
hdl21/prefix.py	`96.53% <0.00%> (-0.44%)`	:arrow_down:
pdks/Sky130/sky130/pdk_data.py	`100.00% <ø> (ø)`
setup.py	`0.00% <0.00%> (ø)`
pdks/Sky130/sky130/pdk_logic.py	`84.26% <40.00%> (+0.29%)`	:arrow_up:
hdl21/tests/test_exports.py	`96.69% <50.00%> (-3.31%)`	:arrow_down:
hdl21/proto/to_proto.py	`86.51% <66.66%> (-0.07%)`	:arrow_down:
hdl21/sim/to_proto.py	`75.91% <66.66%> (+1.04%)`	:arrow_up:
hdl21/scalar.py	`85.18% <80.95%> (+21.05%)`	:arrow_up:
hdl21/bundle.py	`80.56% <83.33%> (+7.38%)`	:arrow_up:
hdl21/params.py	`93.33% <84.00%> (-2.42%)`	:arrow_down:
... and 15 more

... and 1 file with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

dan-fritchman commented 1 year ago

@ThomasPluck - please check out the changes to Scalar here: https://github.com/dan-fritchman/Hdl21/blob/e060483c1289c64a4db3a77af97f48fc2a322710/hdl21/scalar.py

This is how I would prefer that Scalar type/ "thing" to work - more like Union[Prefixed, Literal] than "wrapper thereof".
It means that all the call sites throughout PDKs like this:

@h.paramclass
class Params:
  xyx = h.Param(dtype=h.Scalar, desc="", default=h.Scalar(inner=1 * µ))

Need to become:

@h.paramclass
class Params:
  xyx = h.Param(dtype=h.Scalar, desc="", default=1 * µ)

(Note the latter version always worked; now just have to use it; Scalar is not a constructible thing.)

I know you've got PDK changes brewing. This should just effect those call-sites - presumably all of which are parameter-defaults - and nothing else. (I think.)

You ready for this to go in?
Or wanna get PDK changes merged first?

ThomasPluck commented 1 year ago

Sure, this can go in and I'll refactor Scalar calls.

ThomasPluck commented 1 year ago

Error discovered in a couple tests having scrubbed out h.Scalar calls:

        if not isinstance(orig, h.Scalar):
>           raise TypeError(f"Invalid Scalar parameter {orig}")
E           TypeError: Invalid Scalar parameter number=Decimal('0.42') prefix=<Prefix.MICRO: -6>

Is this something that you'd like to build an exception for, or would you prefer to keep this type definition strict?

dan-fritchman commented 1 year ago

Yep, I noticed the same, example here: https://github.com/dan-fritchman/Hdl21/blob/1cf539ed660c7c4ce5ae58cd169004206d72a497/pdks/Sky130/sky130/pdk_logic.py#L369

I think that's the right approach, and consistent with the idea that Scalar "is" Union[Prefixed, Literal].

dan-fritchman / Hdl21

#140 #76 #30 #151 #148

Codecov Report