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 Size symbol #50

Merged
merged 3 commits into from
Aug 15, 2024
Merged

Add Size symbol #50

merged 3 commits into from
Aug 15, 2024

Conversation

arcondello
Copy link
Member

Closes #48

@arcondello arcondello added the enhancement New feature or request label Jul 11, 2024
@arcondello
Copy link
Member Author

I am toying with renaming these to Length and LengthNode. Need to think about it but wanted to get the logic up to be tested with.

@arcondello arcondello marked this pull request as ready for review July 11, 2024 01:20
@arcondello
Copy link
Member Author

Still ruminating on this, but a few thoughts.

One issue with calling it Len or Length is that the len() of a NumPy array is the size of the first dimension. I am worried that would be confusing.

I am currently inclined to do something like

s: Size = a.size(symbol=True)

It's ugly, but nicely explicit.

Alternatives

  • We could make ArraySymbol.size() always return a symbol, but there are many legitimate uses that want an integer.
  • If we had Consider adding as_symbol() method or similar #38 we could consider having ArraySymbol.size() return a symbol when it's dynamic and an int otherwise. But that's pretty ugly switching behavior.

@arcondello
Copy link
Member Author

arcondello commented Jul 15, 2024

Ah, if we implement __index__() for Constant then returning a constant always from size() will work in most places. Interesting.

Something like

def __index__(self) -> int:
    return int(np.asarray(self))

would let us fall back on NumPy's error message albeit at the cost of some extra object creations. Or we could just copy their error message.

@arcondello
Copy link
Member Author

With the addition of #72, we could now always return a symbol from .size(). Though another downside of that approach is that without some sort of constant caching or de-duplication scheme we might end up with many constants in the model. So returning a symbol only when dynamic might still be preferable.

@arcondello arcondello force-pushed the feature/Len branch 2 times, most recently from 9cfd9cc to 80177f3 Compare August 12, 2024 14:49
@arcondello arcondello changed the title Add Len symbol Add Size symbol Aug 12, 2024
@arcondello
Copy link
Member Author

Ended up going with the approach where we return a symbol only for dynamic nodes. I hate the switching behavior of the return type, but I think it's the least confusion option for now.

@@ -174,7 +174,7 @@ class ArraySymbol(Symbol):
def prod(self) -> Prod: ...
def reshape(self, shape: _ShapeLike) -> Reshape: ...
def shape(self) -> typing.Tuple[int, ...]: ...
def size(self) -> int: ...
def size(self) -> typng.Union[int, Size]: ...
Copy link
Contributor

Choose a reason for hiding this comment

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

typo


void initialize_state(State& state) const override;

// LenNode's value is always a non-negative integer.
Copy link
Contributor

Choose a reason for hiding this comment

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

LenNode => SizeNode ?

yield b
yield c

def test_dynamic(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to get the length for disjoint_lists:

model = Model()
model.states.resize(1)

x, ys = model.disjoint_lists(5, 3)
length0 = ys[0].size()
length1 = ys[1].size()
length2 = ys[2].size()

x.set_state(0, [[0, 1, 2, 3, 4], [], []])
with model.lock():
    print(length0.state(0))
    print(length1.state(0))
    print(length2.state(0))

Is there a better way for doing this? For example, can we make an array of all lengths?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently no way to join them into another array unfortunately, we'd need #46 for that I think

@arcondello arcondello merged commit 96898dc into dwavesystems:main Aug 15, 2024
32 checks passed
@arcondello arcondello deleted the feature/Len branch August 15, 2024 15:54
@arcondello arcondello mentioned this pull request Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider adding a Counter Node for counting the number of elements in a disjoint_list
2 participants