Skip to content

[ fix #395 ] creating Data.X.Solver #400

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

Merged
merged 7 commits into from
Aug 17, 2018
Merged

[ fix #395 ] creating Data.X.Solver #400

merged 7 commits into from
Aug 17, 2018

Conversation

MatthewDaggitt
Copy link
Contributor

  1. Tidied up the Solver hierarchy, partially addressing Fix hyphenated module names #265
  2. Moved solvers from Data.X.Properties to new module Data.X.Solver addressing Dependency cycles involving Solver modules #395
  3. Came up with a meaningful naming convention for the solvers themselves (previously the naming was inconsistent and uninformative).

Comments welcome.

Algebra.Monoid-solver ↦ Algebra.Solver.Monoid
Algebra.CommutativeMonoidSolver ↦ Algebra.Solver.CommutativeMonoidx
Algebra.IdempotentCommutativeMonoidSolver ↦ Algebra.Solver.IdempotentCommutativeMonoid
```
Copy link
Member

@gallais gallais Jul 28, 2018

Choose a reason for hiding this comment

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

Given that we went for Data.X.Categorical rather than Category.X,
wouldn't it make sense to go for Algebra.X.Solver? Or even
Algebra.Structures.X.Solver given that all of these are structures.

It would also be more consistent with the Data.X.Solver naming scheme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's tricky, we're not very consistent between the "big three" (i.e. Data, Algebra and Relation). The current naming scheme is consistent within the Algebra hierarchy: Algebra.Operations.X, Algebra.Properties.X etc.

If we were to adopt your suggestion, then we should do so for all the other Algebra subfolders as well... That does seem like a big change though?

Copy link
Member

Choose a reason for hiding this comment

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

Oh I had not realised that was the case. Better leave it as it is for the moment then!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, we'll leave it for the mythical version 2.0 😆

@MatthewDaggitt
Copy link
Contributor Author

Oh one more thing I forgot I left unresolved. There's a solver in Function.Related.TypeIsomorphisms that I don't really know where to put:

module Solver s {ℓ} =
Algebra.RingSolver.Natural-coefficients
(×⊎-CommutativeSemiring s ℓ)
(coefficient-dec s ℓ)

Thoughts on Function.Related.TypeIsomorphism.Solver?

@gallais
Copy link
Member

gallais commented Jul 28, 2018

Congrats on #400 btw :D

@MatthewDaggitt
Copy link
Contributor Author

Congrats on #400 btw :D

I feel like I'm doing something wrong! There were only 50 issues in the first 10 years of the library, followed by 350 in the past 2....

@JacquesCarette
Copy link
Contributor

No, you're just using the issue system as designed. And the number of contributors has increased, as has the size of the library. This is normal.

@JacquesCarette
Copy link
Contributor

And on the solver name - Function.Related.TypeIsomorphism.Solver is fine (it's consistent).

If I had my way, I would have called the whole file Types.Isomorphisms (since it really have little to do with functions). But it's too late for that.

@MatthewDaggitt
Copy link
Contributor Author

Added the solver file for TypeIsomorphisms

@MatthewDaggitt MatthewDaggitt merged commit f0cec64 into master Aug 17, 2018
@MatthewDaggitt MatthewDaggitt deleted the Solver branch August 17, 2018 15:35
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.

3 participants