FEAT: Convert numeric to prefixed value function

daquintero commented 1 year ago

Hi Dan,

Hope you are well! I find this function useful and maybe it could be handy for others.

It works like:

value = 1e4
converted_value = h.prefix.convert_numeric_to_prefix(value)
assert 10 * h.prefix.K == converted_value
value = 1e-4
converted_value = h.prefix.convert_numeric_to_prefix(value)
assert 0.1 * h.prefix.m == converted_value

I've included this in a test too. Just in case it is of any use.

ThomasPluck commented 1 year ago

Hey, thanks for the contribution!

So I'm comparing your function to one that does something pretty similar called hdl21.prefix.to_prefixed, the Prefixed type is designed to be "Prefix-agnostic" - so it just passes the Decimal'd version of whatever float you give it to a Prefixed with a default prefix of UNIT.

However, it doesn't scale the decimal to the most probable choice Prefix, as your feature does here. I do actually remember this bothering me during #79 and for some reason I dropped it - probably out of laziness.

So, two things that I'd recommend before pulling this:

[ ] I'd extend to_prefixed to use the scaling that you propose using the scale method in Prefixed and closest method in Prefix
[ ] Add a __call__ method to Prefixed to override the default behavior of the pydantic BaseModel to allow Prefixed to allow it behave more intuitively. Example below:

from hdl21 import Prefixed
from hdl21.prefix import MILLI

a = Prefixed(0.001234)
assert a == Prefixed(number=Decimal('1.234'),prefix=MILLI)

daquintero commented 1 year ago

Sounds good! I'll send an updated one

codecov[bot] commented 1 year ago

Codecov Report

Merging #159 (b37fd4a) into main (137333c) will increase coverage by 0.02%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #159      +/-   ##
==========================================
+ Coverage   87.19%   87.22%   +0.02%     
==========================================
  Files         109      109              
  Lines        9568     9590      +22     
==========================================
+ Hits         8343     8365      +22     
  Misses       1225     1225

Impacted Files	Coverage Δ
hdl21/prefix.py	`96.74% <100.00%> (+0.21%)`	:arrow_up:
hdl21/tests/test_prefix.py	`100.00% <100.00%> (ø)`

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

dan-fritchman commented 1 year ago

Looking great! A few thoughts:

Agree there's a good amount of overlap with to_prefixed, as well as Prefixed.scale and Prefix.closest
Maybe make, like, Prefixed.normalize() (or regularize() or something?). Something that takes a Prefixed value, figures out the "preferable" Prefix, and Prefixed.scale()s to that. Idunno what the most typical name for that process would be.
And maybe make "whether to do that" a (boolean) argument to to_prefixed().
Or just don't, and you just need two calls, e.g. to_prefixed(1e-6).regularize()
Definitely don't think we need to restate the list of Prefix values; it's there in the enum definition

Related to, but not part of this PR -

def _scale_to_smaller(
    me: Prefixed, other: Union[Prefixed, ToPrefixed]
) -> Tuple[Prefixed, Prefixed]:
    """# Scale two `Prefixed` numbers to the smaller of the two prefixes.
    The `me` argument is always a `Prefixed`, generally due to being the `self` in a `Prefixed` mthod.
    The `other` argument is commonly another compatible/ convertible type,
    and is converted before scaling."""
    other = to_prefixed(other)
    smaller = me.prefix if me.prefix.value < other.prefix.value else other.prefix
    return me.scale(smaller), other.scale(smaller)

It seems that, without such "normalization"/ "regularization"/ whatever-it's-called... this can be wrong?
E.g. if me is 1_000_000_000 * NANO and other is 1 * MILLI, or similar.

ThomasPluck commented 1 year ago

Not sure there is a dedicated term for picking the common-sense SI prefix, guess you could go with "autoscale"
That is a goofy code snippet, I'll roll in a fix with this PR

daquintero commented 1 year ago

All sounds great and cheers for all your work on this really good software. Sorry, I am away of my computer until next week or would help sooner!

Sent from Outlook for iOShttps://aka.ms/o0ukef

From: Thomas Pluck @.> Sent: Thursday, July 20, 2023 8:31:39 PM To: dan-fritchman/Hdl21 @.> Cc: Dario Antonio Quintero Dominguez @.>; Author @.> Subject: Re: [dan-fritchman/Hdl21] FEAT: Convert numeric to prefixed value function (PR #159)

Not sure there is a dedicated term for picking the common-sense SI prefix, guess you could go with "autoscale"
That is a goofy code snippet, I'll roll in a fix with this PR

— Reply to this email directly, view it on GitHubhttps://github.com/dan-fritchman/Hdl21/pull/159#issuecomment-1644483044, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AIO5PRGQ75TG5DOJGCTHKTLXRGBRXANCNFSM6AAAAAA2M55WBM. You are receiving this because you authored the thread.Message ID: @.***>

dan-fritchman / Hdl21

FEAT: Convert numeric to prefixed value function #159

Codecov Report