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

ScalarParam, ScalarOption undesirably coercing to int #42

Closed
dan-fritchman opened this issue Oct 14, 2022 · 1 comment
Closed

ScalarParam, ScalarOption undesirably coercing to int #42

dan-fritchman opened this issue Oct 14, 2022 · 1 comment

Comments

@dan-fritchman
Copy link
Owner

dan-fritchman commented Oct 14, 2022

Once upon a time, I wrote this long and detailed comment on some of the core choices for the Prefixed type:

"""
# Note on Numeric Types 

`Prefixed` supports a sole type for its `number` field: the standard library's `Decimal`. 
One might wonder why it does not include `int` and `float` as well, or perhaps 
some other numeric-like types. 

This boils down to limitations in validation. Previous versions of `Prefixed` have 
used a `Number` union-type along these lines:
`
Number = Union[Decimal, float, int]
`
This is nominally handy, but results in many values being converted among these types, 
often where one may not expect. 
The pydantic docs are clear about their limitations in this respect: 
https://pydantic-docs.helpmanual.io/usage/types/#unions

These limitations include the fact that despite being declared in list-like 
"ordered" syntax, union-types do not have orders in the Python standard library. 
So even interpreting `Union[Decimal, float, int]` as "prefer Decimal, use float 
and then int if that (for whatever reason) doesn't work" fails, 
since the `Union` syntax can be reordered arbitrarily by the language. 

The other clear alternative is not doing runtime type-validation of `Prefixed`, 
or of classes which instantiate them. Prior versions also tried this, 
to fairly confounding results. Incorporating standard-library `dataclasses` 
as members of larger `pydantic.dataclasses` seems to *work* - i.e. it does not 
produce `ValidationError`s or similar - but ultimately with enough of them, 
triggers some highly inscrutable errors in the standard-library methods. 

So: all `Decimal`, all `pydantic.dataclasses`.
"""

Turns out we failed to apply that to many of the Primitive parameters.
In particular they use a union-type called ScalarParam, which is:

# Type alias for many scalar parameters
ScalarParam = Union[Prefixed, int, float, str]

Which, as that comment indicates, turns to int lots of times that you don't want.
@aviralpandey found this in his ADC work. As with most of these subtle translation bugs, it appeared a pain to track down.

@dan-fritchman
Copy link
Owner Author

We can definitely prevent the confusion by making these all Prefixed or Optional[Prefixed].
Required user-level changes would, for now, be to creating a Prefixed one way or another, e.g.:

import hdl21 as h 
from hdl21.prefix import m 
from hdl21.primitives import Vdc

Vdc.Params(dc=1000*m) # A long-ish way to write "one"
Vdc.Params(dc=h.Prefixed(1, h.Prefix.UNIT)) # A longer one 
Vdc.Params(dc=h.Prefixed(1)) # UNIT becomes the default prefix

Ideally we could make conversions into these inline, e.g. with:

Vdc.Params(dc=1) 

Where this gets converted to Prefixed(Decimal(1)) along the way.
Opened an issue with Pydantic to see whether they can support such a thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant