Skip to content
This repository has been archived by the owner on Dec 10, 2024. It is now read-only.

DSL Bring Up 1 #141

Closed
wants to merge 3 commits into from
Closed

DSL Bring Up 1 #141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7acf134
Libs: Make sets mutable
mawildoer Nov 14, 2024
f70a067
Improve unit checking. Avoids error on construction w/ quantities tha…
mawildoer Nov 15, 2024
ef42cd2
Add properties for parameter attributes instead of exclusively proces…
mawildoer Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 23 additions & 8 deletions src/faebryk/core/parameter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -865,17 +865,10 @@ def __init__(
likely_constrained: bool = False, # TODO rename expect_constraits or similiar
):
super().__init__()
if within is not None and not within.units.is_compatible_with(units):
raise ValueError("incompatible units")

if isinstance(within, Range):
within = Ranges(within)

if isinstance(soft_set, Range):
soft_set = Ranges(soft_set)

if not isinstance(units, Unit):
raise TypeError("units must be a Unit")

self.units = units
self.within = within
self.domain = domain
Expand All @@ -884,6 +877,28 @@ def __init__(
self.tolerance_guess = tolerance_guess
self.likely_constrained = likely_constrained

@property
def within(self) -> Ranges | None:
return self._within

@within.setter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you need this because you first make the Param then find out about its within&sofset.
Got to make sure that we take the mutable within & softset into account everywhere. Maybe limiting it with '@assert_once' might be a good idea for the moment,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that idea. It won't work for the front-end because of how overrides currently work in ato.
Maybe this deserves another issue to improve safety around later? No one in their right mind should do this without being quite sure of what they're doing

def within(self, value: Range | Ranges | None):
if value is not None and not value.units.is_compatible_with(self.units):
raise ValueError("incompatible units")
if isinstance(value, Range):
value = Ranges(value)
self._within = value

@property
def soft_set(self) -> Ranges | None:
return self._soft_set

@soft_set.setter
def soft_set(self, value: Range | Ranges | None):
if isinstance(value, Range):
value = Ranges(value)
self._soft_set = value

# Type forwards
type All = ParameterOperatable.All
type NumberLike = ParameterOperatable.NumberLike
Expand Down
1 change: 1 addition & 0 deletions src/faebryk/libs/library/L.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import logging

from faebryk.core.module import Module # noqa: F401
from faebryk.core.moduleinterface import ModuleInterface # noqa: F401
from faebryk.core.node import ( # noqa: F401
InitVar,
Node,
Expand Down
4 changes: 2 additions & 2 deletions src/faebryk/libs/sets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,14 +398,14 @@ def __init__(
self.units = units or min_unit or max_unit
self.range_units = base_units(self.units)

if isinstance(min, Quantity):
if HasUnit.check(min):
num_min = min.to_base_units().magnitude
Comment on lines +401 to 402
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can not work. HasUnit.check checks if something has an units attr, but in the following we act like the variable is a Quantity (.to_base_units)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might been a better IsQuantity check then, or I can duck-type it w/ the attribute.

It was to guard against this issue: #141 (comment)

if not (isinstance(num_min, float) or isinstance(num_min, int)):
raise ValueError("min must be a float or int quantity")
else:
num_min = min

if isinstance(max, Quantity):
if HasUnit.check(max):
num_max = max.to_base_units().magnitude
if not (isinstance(num_max, float) or isinstance(num_max, int)):
raise ValueError("max must be a float or int quantity")
Expand Down
3 changes: 2 additions & 1 deletion src/faebryk/libs/units.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from typing import Any

from pint import Quantity as _Quantity # noqa: F401
from pint import UndefinedUnitError, Unit, UnitRegistry # noqa: F401
from pint import UndefinedUnitError, UnitRegistry # noqa: F401
from pint.util import UnitsContainer as _UnitsContainer

from faebryk.libs.util import cast_assert
Expand All @@ -14,6 +14,7 @@

UnitsContainer = _UnitsContainer | str
Quantity = P.Quantity
Unit = P.Unit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is different than above. The types are not the same.
Got to be careful here, this might break stuff. Check where Unit is used atm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, changed this because it was breaking stuff in the front-end

[pseudocode]

from ... import Unit, Quantity 

def _get_unit(name: str) -> Unit:
   return Unit(name)
   
assert isinstanc(10 * _get_unit("mV"), Quantity)  # <-- fails

^ This (distilled down) is what'd fail in the front-end because the Quantity and Unit were from different unit registrys

dimensionless = cast_assert(Unit, P.dimensionless)


Expand Down
9 changes: 8 additions & 1 deletion src/faebryk/libs/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -957,7 +957,7 @@ def debugging() -> bool:
return debugpy.is_client_connected()


class FuncSet[T, H: Hashable](collections.abc.Set[T]):
class FuncSet[T, H: Hashable](collections.abc.MutableSet[T]):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you are trying to implement is a custom eq function instead of a custom hasher that is removing collisions.
Just add an extra arg eq that checks if two elements (with the same hash) are equivalent and if so dont add it to the set.

Then no need for the discard stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was using discard in the DSL front-end to keep track of the params that'd been referenced w/o declaration.
Maybe there's something else fucky with this implementation?

"""
A set by pre-processing the objects with the hasher function.
"""
Expand All @@ -973,6 +973,13 @@ def add(self, item: T):
if item not in self._deref[self._hasher(item)]:
self._deref[self._hasher(item)].append(item)

def discard(self, item: T):
hashed = self._hasher(item)
if hashed in self._deref and item in self._deref[hashed]:
self._deref[hashed].remove(item)
if not self._deref[hashed]:
del self._deref[hashed]

def __contains__(self, item: T):
return item in self._deref[self._hasher(item)]

Expand Down
9 changes: 9 additions & 0 deletions test/libs/test_util_func_collections.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,3 +80,12 @@ def test_func_set_hash_collision():
assert len(a) == 2
assert 1 in a
assert 2 in a


def test_func_set_discard():
a = FuncSet((1,2), hasher=lambda x: x)
assert 1 in a
assert 2 in a
a.discard(1)
assert 1 not in a
assert 2 in a
Loading