Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add Uniform and UniformRM instances. #183

Merged
merged 3 commits into from
Jan 24, 2025
Merged

Conversation

vladov3000
Copy link
Contributor

Resolves #182.

@RyanGlScott
Copy link
Collaborator

Thanks for the PR!

This compiles with random-1.3.*, but it does not compile with random-1.2.*, as seen in the CI. This is because the uniformRM method of UniformRange didn't receive a Generic-based default until random-1.3.0.

As the ecosystem is still catching up with random-1.3.*, I think it would be worthwhile to provide compatibility with random-1.2.* by explicitly implementing uniformRM in each instance. Would you be willing to do this? I don't think it would be too difficult to do—for instance, I believe this implementation would suffice for V2:

instance UniformRange a => UniformRange (V2 a) where
  uniformRM (V2 x1 y1, V2 x2 y2) g = V2 <$> uniformRM (x1, x2) g <*> uniformRM (y1, y2) g

Also, the Uniform and UniformRange classes were introduced in random-1.2.*, so make sure to bump the lower version bounds on random to >= 1.2 in the .cabal file.

@Taneb
Copy link
Collaborator

Taneb commented Jan 23, 2025

How hard would it be to also add instances for Linear.V.V?

@vladov3000
Copy link
Contributor Author

In my new commit, I added explicit uniformRM implementations and bumped up the version of random. I also added an instance of Uniform and UniformRM for Linear.V.V.

@vladov3000
Copy link
Contributor Author

Also, we need conditional compilation of the isInRange method which was only added in random-1.3.0. I built with both a 1.2 and 1.3 version to check that it works. Why do you set the default value of MIN_VERSION_* macros to 1?

Copy link
Collaborator

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good to me.

Why do you set the default value of MIN_VERSION_* macros to 1?

Judging from fbef54e, I think this was a technique that was required for old versions of HLint and when trying to compile the library code with bare GHC (way back when that was a somewhat common thing to do). Nowadays, HLint can handle CPP just fine, and building library code with a build tool like cabal is pretty much the standard. As such, we could probably remove these MIN_VERSION_* macro redefinitions. (No need to do so as part of this PR, however. I can do that separately.)

@RyanGlScott RyanGlScott merged commit 14cbaad into ekmett:master Jan 24, 2025
13 checks passed
RyanGlScott added a commit that referenced this pull request Jan 25, 2025
RyanGlScott added a commit that referenced this pull request Jan 25, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

V2 is not an instance of Uniform or UniformRange.
3 participants