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

Tests.QuickCheckUtils: refactors #374

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Anton-Latukha
Copy link

@Anton-Latukha Anton-Latukha commented Oct 3, 2021

The main agenda of the patch is to refactor the code of Arbitrary {,L}Text generation.

Then, because there were a lot of functoring of newtype accessors, went over a module and applied coerce.

Tests.QuickCheckUtils: i Arbitrary (NotEmpty {,L}Text): refact a2a2179 is put separately, since: generally prefer to already organize code used 2 times, but here it is a bit excessive pyramid for what that code is doing.

@Anton-Latukha Anton-Latukha marked this pull request as ready for review October 3, 2021 08:33
Copy link
Contributor

@Bodigrim Bodigrim left a comment

Choose a reason for hiding this comment

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

@Anton-Latukha thanks! Could you possibly elaborate what is the issue you are solving in this PR? Because generally speaking I prefer something with specific types like fmap (NotEmpty . TL.pack . getNonEmpty) over fmap (coerce . f . coerce). While there are performance reasons to use coerce instead of fmap getNonEmpty, on other occasions I'd prefer to keep concrete constructors/accessors.

tests/Tests/QuickCheckUtils.hs Outdated Show resolved Hide resolved
tests/Tests/QuickCheckUtils.hs Outdated Show resolved Hide resolved
tests/Tests/QuickCheckUtils.hs Outdated Show resolved Hide resolved
tests/Tests/QuickCheckUtils.hs Outdated Show resolved Hide resolved
tests/Tests/QuickCheckUtils.hs Outdated Show resolved Hide resolved
@Anton-Latukha
Copy link
Author

Anton-Latukha commented Oct 4, 2021

Could you possibly elaborate what is the issue you are solving in this PR?

I went to do subsequence tests & so generation, and so - looked into the generation of {,L}Text.

& I love refactoring, a good did, while helps me understand the code and make reading & working with it easier for others.

It is in large a try of how the dialog would go, without opening a report. It is a test suite module, so it is once removed from the active changes. Because I generally do not want to open a report/discussion/code style discussions in core packages, especially on the use of coerce, in a library as text starting that kind of a discussion can make a feedback splash. So it is good to test waters to not stir up the pot, again, this is a test suite module, so generally, people would not complain about some coerce there, while this module has half of newtypes, their instances & rewrapping in the project, so it is a central place to apply coerce in the package.

Because generally speaking I prefer something with specific types like fmap (NotEmpty . TL.pack . getNonEmpty) over fmap (coerce . f . coerce)

Well - that is the thing I was testing. Since I think that the active maintainer determines the code style in the project.

I personally love supplying type signatures to all (even local) functions, so when some change happens - having a solid typing suite to work with, and coerce then are largely the details.

coerce is a great addition to {pure, mempty} arsenal. It is one of the most powerful things I've seen to simplify the code, do transformations & navigate the type system, it is polymorphic, while also completely free. There is no longer a need to concentrate on either that is rewrapper or not and how that rewrapper is called for the type, un.*,get.*,run.*,..., or constructor, and how to compose that, coerce eliminates them, it can easilly transition between quite complex types, which is fully controlled by type-level code. And I love to read the needed type information, not from the constructors/accessor names (execution level code), but from the type signatures and type applications. And from now on GHC/HLS point out the type conversion for coerce. Some people say that from the 90s on, the introduction of a coerce is the biggest language change that happened, to the Haskell core languages for sure.

Completely Ok with some core Haskell project as text keeping the style. Because transition transforms the code by quite a bit, especially also because coerce goes together with ViewPatterns here and there, like even in this file - in the BigInt/Big constructor type in this file.

And already submitted a type error (using Gen [Sqrt (NotEmpty T.Text)] not Gen (Sqrt [NotEmpty T.Text])) while converting the code, so guilty as charged, one can make an error doing the transition.

@Anton-Latukha Anton-Latukha force-pushed the 2021-09-30-refactors branch 2 times, most recently from fcbbf1e to 44af64c Compare October 4, 2021 11:26
@Anton-Latukha
Copy link
Author

So, make a decision, I would comply with it, I can drop any of the commits made. And as I understood, would know not to refactor with coerce in the computation modules.

Copy link
Contributor

@Bodigrim Bodigrim left a comment

Choose a reason for hiding this comment

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

Strictly speaking, I'd like to discourage refactoring for the sake of refactoring, without specific tangible benefits. I also strongly suggest to raise an issue first and get at least one of maintainers on board before preparing a PR (unless it is a trivial typo or life cycling).

But since we already spent time to review, let's make a one-off exception and grasp a possibility to polish this module even further.

shrink = map Sqrt . shrink . unSqrt
arbitrary = coerce $ sized $ \n -> resize (smallish n) $ arbitrary @a
where
smallish = round . (sqrt :: Double -> Double) . fromIntegral . abs
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be a perfect use case for @phadej's approximate square root. Note that one can avoid division and remain entirely in the realm of bit fiddling by something along the following lines:

intSqrt :: Int -> Int
intSqrt n = if n < 2 then n else let b2 = shiftR (finiteBitSize n - countLeadingZeros n) 1 in shiftR (shiftL 1 b2 + shiftR n b2) 1 

Copy link
Author

@Anton-Latukha Anton-Latukha Oct 5, 2021

Choose a reason for hiding this comment

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

Yes. Binary division can be replaced with bit shifting.

First, I should understand and test this particular implementation myself, before putting it into the code.

But it is a place where it starts to feel as overengineering. My work was mainly to iso the code improving code quality in elegance/readability, so that the code is easier to work with, some performance gains are a byproduct of that. To do a custom code for RAM bit shifting to optimize testsuite times seems a bit too much.

But a nice idea overall. If intSqrt is really needed, to do it properly, it should be included into libraries. A nice thing to contribute upstream.

Of course, you have https://hackage.haskell.org/package/integer-roots-1.0.0.1/docs/src/Math.NumberTheory.Roots.Squares.html#integerSquareRoot.

So far I not done this. So now PR-review status goes to "not finished".

tests/Tests/QuickCheckUtils.hs Outdated Show resolved Hide resolved
tests/Tests/QuickCheckUtils.hs Outdated Show resolved Hide resolved

instance Arbitrary (Precision Double) where
arbitrary = arbitraryPrecision 22
shrink = map Precision . shrink . precision undefined
shrink = coerce . shrink . precision undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we somehow avoid undefined smell here and above?

Copy link
Author

@Anton-Latukha Anton-Latukha Oct 5, 2021

Choose a reason for hiding this comment

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

precision had a redundant type witness, it was just yet another manual implementation of coerce.

As function is exported - instead of removing, placed a deprecation message and a date.

tests/Tests/QuickCheckUtils.hs Outdated Show resolved Hide resolved
@Lysxia
Copy link
Contributor

Lysxia commented Oct 5, 2021

The motivation for these stylistic changes is quite subjective, it doesn't seem strong enough to merge this PR.

@Anton-Latukha
Copy link
Author

Anton-Latukha commented Oct 5, 2021

The motivation for these stylistic changes is quite subjective, it doesn't seem strong enough to merge this PR.

@Lysxia It is anti-social to try to block the PRs of new contributors.

Best practices in the programming community are formulated quite nicely.

You do your work, I do mine. We did mistakes establishing a dialog in #369 (comment). I personally do not have any problem with you nor with your positions.

@Bodigrim
Copy link
Contributor

Bodigrim commented Oct 5, 2021

@Anton-Latukha your reaction to #374 (comment) is out ot proportion. @Lysxia is entitled to have an opinion and expressed it in the most neutral form. Being a new contributor does not make your PR immune to potential rejection. If you are unhappy with this approach (e. g., you find it detrimental for community growth), please raise the matter directly with CLC.

As I already said in #374 (review), refactoring for the sake of refactoring is unwelcome. Indeed, as a gesture to a new contributor, I (as one of several maintainers) am still open to consider this PR. However, doubling down with stylistic changes as in 8f6ed3b will not bring us anywhere.

@Lysxia
Copy link
Contributor

Lysxia commented Oct 5, 2021

@Anton-Latukha It is unfortunate that we started with two disagreements, but I have nothing against you personally either. It is my job to vet changes, and I voiced my opinion early and firmly so that other maintainers can react if they disagree, and otherwise to incite you to redirect your efforts on another issue if you're looking to make a contribution.

@Anton-Latukha
Copy link
Author

Ok.

The layouting in the module was highly inconsistent, since I looked at it & it seemed completely random mix - started to do mine one, pretty simple diagonal layouting. But ironically for refactoring.

I would roll it back

But if use of coerce is not encouraged - lets just close this.

@Bodigrim
Copy link
Contributor

@Anton-Latukha sorry for silence, I'm too busy to respond properly. From my perspective, more active use of coerce (especially when it leads to non-trivial runtime improvements) is fine, while other refactorings are better to be delayed.
CC @Lysxia @Boarders @parsonsmatt

@Anton-Latukha Anton-Latukha force-pushed the 2021-09-30-refactors branch 2 times, most recently from a7fbc36 to 594d777 Compare November 2, 2021 17:36
@Lysxia Lysxia added the internal No API-level changes label May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal No API-level changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants