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

feat: cleanup FSM code to contain real abstractions instead of directly applying low-level functions. #667

Closed
wants to merge 32 commits into from

Conversation

bollu
Copy link
Collaborator

@bollu bollu commented Sep 29, 2024

No description provided.

Comment on lines 62 to 63
internal carry state of this FSM has -/
( σ : Type ) -- Why is σ also not an index?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
internal carry state of this FSM has -/
( σ : Type ) -- Why is σ also not an index?
internal carry state of this FSM has. -/
( σ : Type ) -- Why is σ also not an index?

Comment on lines 89 to 90
a finite bitvector whose width is given by the arity of `p.σ` -/
abbrev State (p : FSM arity): Type := BoolProd p.σ
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
a finite bitvector whose width is given by the arity of `p.σ` -/
abbrev State (p : FSM arity): Type := BoolProd p.σ
a finite bitvector whose width is given by the arity of `p.σ`. -/
abbrev State (p : FSM arity): Type := BoolProd p.σ

Comment on lines 101 to 103


end FSM
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
end FSM
end FSM

Comment on lines 134 to 136


def StateStream (p : FSM arity) := ℕ → p.State
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def StateStream (p : FSM arity) := ℕ → p.State
def StateStream (p : FSM arity) := ℕ → p.State

Comment on lines 138 to 139
/-- `p.carryStream inputStream` computes the stream of carries, from the stream of inputs -/
def carryStream (inputStream : BitStreamProd arity) : p.StateStream := fun n =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/-- `p.carryStream inputStream` computes the stream of carries, from the stream of inputs -/
def carryStream (inputStream : BitStreamProd arity) : p.StateStream := fun n =>
/-- `p.carryStream inputStream` computes the stream of carries, from the stream of inputs. -/
def carryStream (inputStream : BitStreamProd arity) : p.StateStream := fun n =>

Comment on lines 16 to 17
/-- An `arity` indexed product of Booleans. -/
def BoolProd (arity : Type) : Type := arity → Bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're going to refactor this, I think I'd prefer to just pay the cost once, change arity to a Nat, and use BitVec w

fun a => (x a).eval y

/-- A 'arity' indexed product of `Bitstream`s. -/
def BitStreamProd (arity : Type) : Type := arity → BitStream
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def BitStreamProd (arity : Type) : Type := arity → BitStream
def BitStreams (arity : Nat) : Type := Fin arity → BitStream

Copy link
Collaborator

Choose a reason for hiding this comment

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

Although I fully admit I'm being self-contradictory, given my previous crusade against semantically similar definitions having a <= 1 edit distance, somehow singular vs plural feels like a safe exception to that rule.

Otherwise, I'd be more comfortable even calling it a BitStreamVec(tor) --- although that is tempting confusion with BitVectors (aagh, why is naming so hard). In my mind, Prod means binary product, an n-ary product is a Vector

Comment on lines 49 to 53
def BitStreamProd.head (x : BitStreamProd arity) : BoolProd arity :=
fun i => (x i).head

def BitStreamProd.tail (x : BitStreamProd arity) : BitStreamProd arity :=
fun a => (x a).tail
Copy link
Collaborator

Choose a reason for hiding this comment

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

These could use some docstring explicitly explaining you're doing head/tail on the constituent bitstreams! I was a bit confused by the arity being the same on both sides at first.

@bollu bollu changed the title Fsm abstractions feat: cleanup FSM code to contain real abstractions instead of directly applying low-level functions. Sep 30, 2024
Copy link

github-actions bot commented Oct 3, 2024

Alive Statistics: 76 / 93 (17 failed)

Let's not continue this
Copy link

github-actions bot commented Oct 3, 2024

Alive Statistics: 76 / 93 (17 failed)

Co-authored-by: Siddharth <[email protected]>
Co-authored-by: Luisa Cicolini <[email protected]>
Copy link

github-actions bot commented Oct 3, 2024

Alive Statistics: 76 / 93 (17 failed)

Copy link

github-actions bot commented Oct 4, 2024

Alive Statistics: 76 / 93 (17 failed)

Copy link

github-actions bot commented Oct 5, 2024

Alive Statistics: 76 / 93 (17 failed)

Copy link

github-actions bot commented Oct 6, 2024

Alive Statistics: 76 / 93 (17 failed)

Copy link

github-actions bot commented Oct 7, 2024

Alive Statistics: 76 / 93 (17 failed)

Copy link

github-actions bot commented Oct 7, 2024

Alive Statistics: 76 / 93 (17 failed)

1 similar comment
Copy link

github-actions bot commented Oct 7, 2024

Alive Statistics: 76 / 93 (17 failed)

Copy link

github-actions bot commented Oct 7, 2024

Alive Statistics: 76 / 93 (17 failed)

Copy link

github-actions bot commented Oct 8, 2024

Alive Statistics: 76 / 93 (17 failed)

If the set of states R is closed under the inverse image of next state,
and if every state in the set R produces a true next output,
and furthermore, if our initial state is not R,
the the value at the initial state is zero, and furthermore, the state will not enter into R in the next state
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
the the value at the initial state is zero, and furthermore, the state will not enter into R in the next state
the value at the initial state is zero, and furthermore, the state will not enter into R in the next state

Copy link

github-actions bot commented Oct 8, 2024

Alive Statistics: 76 / 93 (17 failed)

Copy link

github-actions bot commented Oct 8, 2024

Alive Statistics: 76 / 93 (17 failed)

Copy link

Alive Statistics: 76 / 93 (17 failed)

Copy link

Alive Statistics: 76 / 93 (17 failed)

Copy link

Alive Statistics: 76 / 93 (17 failed)

Copy link

Alive Statistics: 76 / 93 (17 failed)

Copy link

Alive Statistics: 76 / 93 (17 failed)

1 similar comment
Copy link

Alive Statistics: 76 / 93 (17 failed)

@tobiasgrosser
Copy link
Collaborator

This has now been superseeded, I guess.

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

Successfully merging this pull request may close these issues.

4 participants