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

Cleaning up dependencies on Data.Fin and Data.Nat ahead of a first attempt to fix Issue1686 #1830

Merged
merged 57 commits into from
Oct 7, 2022

Conversation

jamesmckinna
Copy link
Contributor

@jamesmckinna jamesmckinna commented Sep 20, 2022

Along the lines of the 'worker/wrapper' distinction I made in the discussion of the issue.

Outstanding task(s):

  • CHANGELOG, as ever;
  • Algebra.Properties.CommutativeMonoid.Sum
  • Data.Fin.Base
  • Data.Fin.Properties
  • Data.List.Properties
  • Data.Vec.Base
  • Data.Vec.Functional
  • Data.Vec.Properties
  • Data.Vec.Recursive
  • Data.Vec.Relation.Unary.All
  • Data.Vec.Relation.Unary.Any
  • Data.Vec.Relation.Unary.Linked.Properties

EDITED: These are now both pending, given Matthew's comments below.
UPDATED: the above task list shows all (at least) the files needing to be touched by the PR
I'd like to merge the existing changes first before proceeding to working on them.

@jamesmckinna jamesmckinna changed the title A first attempt to fix Issue1686 Cleaning up dependencies on Data.Fin and Data.Nat ahead of a first attempt to fix Issue1686 Sep 29, 2022
@jamesmckinna
Copy link
Contributor Author

Hmmm. I think that this is now OK, but I am slightly worried that the previous commits to agda/master which needed to have their merge conflicts resolved, are now part of this PR? Ooops, if so, and I'll try again later.

Otherwise, this PR is the 'part I' alluded to above: namely groundwork preparation (mostly dependency thinning; and no (?) new functions or deprecations) ahead of the more controversial part II, where NonZero-bracketed functions and their properties get propagated throughout. Towards such a move, all the imports of Data.Nat{.Base} now explicitly import NonZero and pred, ahead of their eventual (heavy) use to implement Matthew's proposal. But further discussion of that required, it would seem.

@jamesmckinna
Copy link
Contributor Author

jamesmckinna commented Oct 3, 2022

Following f2f discussion with @MatthewDaggitt today, my revised proposal is to:

  • retain existing functionality as 'worker' functions, with their existing names, reflecting that they are the 'more dependent' (because their types involve inhomogeneous telescopes eg {n : ℕ} (i : Fin (suc n)))
  • in place of the original, ugly, NZ prefix to the above, write 'wrapper' functions with the same names, but primed, for the corresponding homogeneous operations prefixed on a {{NonZero n} instance (because 'less dependent' here = homogeneously telescoped)

I'll push those commits another day, once this proposed naming convention has settled (?).

infixl 6 _ℕ-suc_

_ℕ-suc_ : (n : ℕ) → Fin n → ℕ
suc n ℕ-suc i = n ℕ-ℕ i
Copy link
Contributor

Choose a reason for hiding this comment

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

Difficult to judge the utility of this without the context that it is being used. Any chance we could keep this and nℕ-suci<n to the follow up PR?

Copy link
Contributor Author

@jamesmckinna jamesmckinna Oct 5, 2022

Choose a reason for hiding this comment

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

Matthew,
this function is not currently used, but since at various points in the library there are uses of its (grotesque) definitional equivalent n ∸ suc (toℕ i) I thought it worth lifting out as something in its own right; but admittedly in obscure corners:

  • Data.Fin.Properties.opposite-prop, Data.Fin.Properties.opposite-suc
  • Data.List.Properties.lookup-applyDownFrom, Data.List.Properties.lookup-downFrom

So very easy to prune this, and its properties, back. But it is an example of a 'naturally'-occurring homogeneously-telescoped function on Fin n, so in the spirit of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, in which case could you make use of it in this PR to simplify the code that you mention?

My deepest apologies if this was previously in this PR before and you have to add it back in. I'm just playing the part of Goldilocks. Previously the PR was too big to review, but now it contains too small in that it contains this partial change that doesn't stand on its own. In general, we tend to like each PR to stand on its own as people (including me!) promise to do things, but don't actually come back and do it! Not implying that you will, but as a general rule these things happen! Therefore we like each PR to be an obvious standalone improvement over the existing code. Adding a function that is never used doesn't quite pass this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha! No, it wasn't, more that I had been laying the groundwork for it... much as I had been enriching the imports of Data.Nat with NonZero and pred.

But you make a good point. When I reinstate it, I'll also use it! Or did you want this to happen now, as opposed to later as part of the whole head/head' etc. enrichment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't mind, happy either way. Whichever you'd prefer!

@jamesmckinna
Copy link
Contributor Author

Some more tidying up possible: don't push yet!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants