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

[DRY] why is _≉_ defined both for Algebra.Bundles.Raw.RawX bundles, and via Setoid instances, for Algebra.Bundles.X? #2274

Open
jamesmckinna opened this issue Feb 3, 2024 · 18 comments

Comments

@jamesmckinna
Copy link
Contributor

jamesmckinna commented Feb 3, 2024

This is another instance of the issues arising when adding 'derived forms' to a(n algebraic) structure/bundle, namely where should they be added:

The first and third cases seem to me to represent a clear DRY violation, and probably arise from the accumulation of 'legacy' design decisions. But what, if any, should be our refactoring policy towards a revised design?

Context: I'm working on a branch/PR addressing #2273 and find myself wondering whether (Raw)SuccessorSet needs explicitly to declare a negated equality symbol, when it should be (available to be) brought into scope via the Setoid instance of IsSuccessorSet...

UPDATED I've tried to regularise my vocabulary to avoid speaking of 'instance's of record types in favour of referring to their (often also record-typed`) fields, also often manifest, ie. defined rather than declared.

@jamesmckinna
Copy link
Contributor Author

In the absence of further discussion yet, note there are two possible scenarios:

I'm not sure what the official (universal algebra) nomenclature is for each kind, but on the model of the theory of formal systems, I supposed the first should be called 'derivable', while the second 'admissible'?

In any case, this issue concerns derivable operations being defined 'too late' to be exported to any subsequently definable extension of the algebra. And that does seem to be a DRY problem... over and above the 'where should such things live?' question...

@JacquesCarette
Copy link
Contributor

Finally catching up on really old stuff - my mailbox is quite full of saved email about items that I meant to comment on. Still have 30 or so from 2024 alone, never mind a whole bunch from 2023!

I do agree that the library is over-eager to define 'helper' conservative extensions that turn out to be seldom used. (I don't know what the official nomenclature is either, but 'derivable' and 'admissible' make a lot of sense.)

I'm a bit concerned about solving DRY problems by over-defining things just on the off chance they might be needed. (Especially here as negation is so so often a code smell in constructive mathematics.)

Personally I'd prefer some 'helper' module(s) [with a better name than Helper !] that define extensions in a standard way. I've migrate things towards that in agda-categories -- it has really helped reduce bloat. (Monoidal was criminally bloated, which had non-trivial downstream effects.)

@jamesmckinna
Copy link
Contributor Author

jamesmckinna commented May 7, 2024

@JacquesCarette thanks for returning to this! And I appreciate you separating out two parts to this:

  • what notions should be added automatically?
  • where should they be added?

But here the problem is that, independent of any answer to the first, the second is answered in weird/bad/DRY-violating ways. So from my point of view it's not a case of being "over-eager", except inasmuch as contributors have not been clear what to add where, so have ended up duplicating structure... and in the case of RawX bundles, they are a relatively recent addition, so a certain amount of knock-on refactoring might have been anticipated/expected, but has not (yet) happened.

As to the first, I'm a bit 'wider' than you as regards extending record APIs, here out of a consideration for consistency of notation, as well as relieving the user of the burden of ad hoc invention, esp. in the context of a 'standard' library (along the lines of 'batteries included': 'if you open this, then you'll get all of this extra, for free'), and I'm willing to take the space hit... I think "bloat" is too strong here!

So a resolution of this issue for me would happily resolve the first question in whatever form (we should reach agreement... either as an 'extension' module, or as 'batteries included', and if so, how much?), but first how we consider appropriate to resolve/standardise the second...

@jamesmckinna
Copy link
Contributor Author

jamesmckinna commented May 7, 2024

As for _≉_ itself, it's perhaps unfortunate to have it in the title of the issue, which partly reflects that the underlying Setoid structures of an Algebra structure/bundle are defined under Relation.Binary, and, moreover, are not defined relative to a Raw (equality) relation (single field records tend not to get defined in the library, except for instance inference, and the Unit type as a special case...?), so it seems as though there isn't an obvious place to add negated equality as an additional manifest field...

... but your point about it being a code smell is an interesting one. Barring eg it's 'general' properties wrt Respects mentioned in #2341 ...

... so a possible 'better' place to add it would/might be as a manifest field of DecidableEquality (where it surely wouldn't be a code smell?) but for the fact that we'd then have to add 'the negation of A' as a manifest field to Dec plus a renaming opening for such objects... but that seems too horrible to contemplate!?

@JacquesCarette
Copy link
Contributor

Through the many many words, talking about many fascinating issues... I've lost the thread about what is the main issue at stake here?

@MatthewDaggitt
Copy link
Contributor

My view is that really the correct solution is number 1. It should be in the Raw bundles. I think the missing part of the puzzle is that we don't have a RawSetoid bundle in which to put the negated equality operator...

@jamesmckinna
Copy link
Contributor Author

Through the many many words, talking about many fascinating issues... I've lost the thread about what is the main issue at stake here?

Yup, that's my weakness... (too) "many many words" ;-)

TL;DR: there are 2 issues at stake:

  • (general) where should admissible/derivable operations live, so that we correctly export them in sub-structures/bundles; in general, I believe that derivable operations should live in the Raw bundle (but IsGroup is an anomaly, so _//_, _\\_ and _-_ should be moved to the RawGroup bundle...)
  • (particular) the derivable _≉_ is an anomaly, for the reasons that @MatthewDaggitt points out (because RawSetoid doesn't exist; another possible downstream PR? in which case where should it live: under Relation.Binary.Bundles.Raw which would be new, or under Algebra.Bundles.Raw)

In each case, the solution would (seem to) be a fiddly (and possibly, for the IsGroup case, breaking) PR moving things around/adding new things, so suggest tagging this as v3.0 when we might reconsider this and eg. #2252 #2255 #2268 (though this could be dealt with independently in v2.1 as a prototype of how to fix it in general...?)

@jamesmckinna jamesmckinna added this to the v3.0 milestone May 15, 2024
@Taneb
Copy link
Member

Taneb commented May 15, 2024

RawSetoid, if we want to add it, should live in Relation.Binary.Bundles.Raw, and it should probably have some friends in there with it

@jamesmckinna
Copy link
Contributor Author

@Taneb yup, all the various ordering relations and their flipped- and negated- versions should be in Raw bundles for such things... so maybe a v2.1 version is possible/desirable?

@jamesmckinna
Copy link
Contributor Author

Looping back to @JacquesCarette 's comment above, in the light of #2491

I'm a bit concerned about solving DRY problems by over-defining things just on the off chance they might be needed. (Especially here as negation is so so often a code smell in constructive mathematics.)

Personally I'd prefer some 'helper' module(s) [with a better name than Helper !] that define extensions in a standard way. I've migrate things towards that in agda-categories -- it has really helped reduce bloat. (Monoidal was criminally bloated, which had non-trivial downstream effects.)

I think that perhaps, along the 'minimalist'/'maximalist' spectrum of library-design, that you might be right in leaning towards 'minimalist'... as regards properties. Here, I'm more concerned with vocabulary ... which feels 'lighter' (as regards 'bloat'), while at the same time providing clients with 'just enough' extra structure that they don't end up reinventing the wheel?

@jamesmckinna
Copy link
Contributor Author

While working on #2491 I had hoped that Relation.Binary.Properties.ApartnessRelation could be refactored to make better use of the negated apartness symbol defined in the Raw bundle... but in fact it needs to be defined in the IsApartnessRelation structure in order to be used in Relation.Binary.Properties.ApartnessRelation.

So @JacquesCarette 's general point about 'helper' modules actually seems to suggest that we should prefer option 2 in the original proposal above, because the 'helper module' associated with an IsX structure (in *.Properties.X) does not depend on the extended Raw bundle... and trying to retrofit that would seem very strange to me... unless we go for the hypothetical-rewrite proposal in #2252 ...!?

@JacquesCarette
Copy link
Contributor

Depending on what you mean by vocabulary, I've moved some of that out too -- see the sub-module ShortHands in Categories.Category.Monoidal.Utilities!

I used to think that defining all sorts of helper stuff early and 'globally' was a great idea, as I thought that this had low 'weight'. Turns out this is not true in current Agda (but could be a deficiency?) So I've moved to pulling these out, at least in the worst offenders.

@jamesmckinna
Copy link
Contributor Author

Thanks @JacquesCarette I think that Matthew has commented elsewhere as to the excess 'weight' of adding properties to IsX records. Interesting that even introducing symbols/derived forms might be undesirable. Of course, we could agree not to merge any PR moving towards the proposal here until this discussion has bottomed-out.

@JacquesCarette
Copy link
Contributor

Coming back to this: adding some small amount of very standard vocabulary might make sense. We'd have to have guidelines. And yes, vocabulary is different than properties.

I was just shocked by how much "standard vocabulary" came with a Monoidal Category -- and how much that bloated things downstream.

@jamesmckinna
Copy link
Contributor Author

Not sure if #2491 closes this, or whether it should be kept open for the above more general discussion, and pending (resolution of) the IsEquivalence vs. Setoid discussion in #2499 (ie whether Algebra.Structures should export (Raw)Setoids at all...)?

@JacquesCarette
Copy link
Contributor

Unfortunately I get the feeling that the issue outlined here isn't quite all resolved (yet). If nothing else, there's some design to document.

@jamesmckinna
Copy link
Contributor Author

jamesmckinna commented Dec 31, 2024

Thanks @JacquesCarette .

I think that there are two things to do to resolve the [ DRY ] aspect, and separately, to leave this issue open regarding the wider question of the minimal/maximal 'basic' API for Algebra:

  • adding RawSetoid etc. and hence moving the question of such API design choices to (potential refactorings of) Algebra.Bundles.Raw; I think Add Raw bundles to Relation.Binary.* hierarchy #2491 does achieve this
  • adding a rawSetoid manifest field to Algebra.Structures.*, and, via open public, bringing the existing negated equality symbol into scope, and leaving open what, if anything, might additionally be brought into scope in a downstream refactoring

Regarding the second, I think this can be done with a minimally invasive PR, in preparation now: #2536

@JacquesCarette
Copy link
Contributor

Yes, I do think that that PR does the job. See additional comments there.

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

No branches or pull requests

4 participants