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

[ fix #870 ] Finish deprecating Data.Table #1078

Closed
wants to merge 2 commits into from
Closed

[ fix #870 ] Finish deprecating Data.Table #1078

wants to merge 2 commits into from

Conversation

gallais
Copy link
Member

@gallais gallais commented Jan 29, 2020

No description provided.

Copy link
Contributor

@MatthewDaggitt MatthewDaggitt left a comment

Choose a reason for hiding this comment

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

Sorry, I thought I'd submitted this review ages ago. Obviously I hadn't!

Basically a few comments about design decisions that I made when adding this that I'd kind of like to take this opportunity to reverse. I'm happy to implement my suggestions if you'd like @gallais ?

sumₜ : ∀ {n} → Table Carrier n → Carrier
sumₜ = Table.foldr _+_ 0#
sumₜ : ∀ {n} → Vector Carrier n → Carrier
sumₜ = Vector.foldr _+_ 0#
Copy link
Contributor

Choose a reason for hiding this comment

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

The name sumₜ probably needs to be changed as the refers to Table?

-- Changes the order of elements in the table according to a permutation (i.e.
-- an 'Inverse' object on the indices).

permute : ∀ {m n a} {A : Set a} → Fin m ↔ Fin n → Vector A n → Vector A m
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd quite like to take this opportunity to correct some sub-optimal design decisions that I think we (well mostly I!) made here.

For starters, the permute function should just take a Fin m → Fin n function rather than the full inverse proof right? This makes it more generally applicable, and we can then prove properties about it depending on what properties the function satisfies? (also we should try and avoid using the old function hierarchy)

-- The result of 'select z i t' takes the value of 'lookup t i' at index 'i',
-- and 'z' everywhere else.

select : ∀ {n} {a} {A : Set a} → A → Fin n → Vector A n → Vector A n
Copy link
Contributor

Choose a reason for hiding this comment

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

We should try to be consistent with the terminology in List and Vec by using the standard _[_]≔_ syntax


module _ (S@record { Carrier = A } : Setoid c ℓ) where

↔Vec : ∀ {n} → Inverse (Pw.setoid S n) (VecPw.setoid S n)
Copy link
Contributor

Choose a reason for hiding this comment

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

Again we should use the new function hierarchy here?

-- There exists an isomorphism between vectors as functions on the one
-- and and vectors as datatypes on the other.

module _ (S@record { Carrier = A } : Setoid c ℓ) where
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooooh that's fancy syntax. Is that new in 2.6.1?

@MatthewDaggitt
Copy link
Contributor

Going to try and have a look at this this morning.

@MatthewDaggitt
Copy link
Contributor

Superseded by #1100

@MatthewDaggitt MatthewDaggitt added status: duplicate The main contents of the issue or PR already exists in another issue or PR. and removed status:not-ready-yet labels Mar 8, 2020
@MatthewDaggitt MatthewDaggitt deleted the issue870 branch September 21, 2020 04:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
admin deprecation status: duplicate The main contents of the issue or PR already exists in another issue or PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants