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

Bring F# PowerPack's Complex & BigRational and FSharpx's DistributionMonad #61

Merged
merged 7 commits into from Dec 5, 2012

Conversation

ovatsus
Copy link
Contributor

@ovatsus ovatsus commented Nov 30, 2012

This is related to #60

We want to not have Math stuff on FSharpx and instead contribute it to Math.Net
The INumerics interface and matrix classes from PowerPack are also interesting to port over, but that needs more work as there's overlap. I'll suggest something in a couple of days

@cdrnet
Copy link
Member

cdrnet commented Dec 1, 2012

Thanks a lot!

Some notes (I can help):

System.Numerics and portable builds
Our portable builds do not currently reference System.Numerics because it is not available on some targets (WP8 and .Net 4.0). Instead we replace them on (and only on) portable builds with equivalent types (Complex, but not yet BigInteger). Luckily the situation has been improved somewhat with .Net 4.5 and VS2012: if we decide to change the portable profile to ".Net 4.5, SL5, Windows Store" and drop WP8 support then we can always reference System.Numerics and get rid of our replacement types. I can take care of that. In order to support BigRational, we need to do either that, or hide BigRational in portable builds. Dropping WP8 is not that a big issue, after all support has just been added in the last release (even though I liked supporting it).

Complex Numbers
We currently use the Complex type from System.Numerics (or our own type replacing it on portable builds, see point above) for double precision, and our own Complex32 type for single precision. See Complex32.cs, Complex64.cs and Trig.cs. So there is some overlap with the complex types from the PowerPack. It seems our type supports all features of the PowerPack one, but the Complex F# module could still be interesting for convenience. What do you think?

Portable Project
The FSharpPortable project would have to be updated as well (files linked directly to normal project). I can take care of that as well.

Distribution Monad
Looks interesting! If I understand it correctly, this monad actually represents a "random variable" instead of a "distribution". Can you confirm this? If yes, we may want to rename it to avoid confusion (since we do provide all kind of standard probability distributions).

Thanks,
Christoph

@ovatsus
Copy link
Contributor Author

ovatsus commented Dec 1, 2012

Updated the pull request with:
-Include files in portable build
-Include implementation of BigInteger from FSharp.Core in portable build
-Revised the Complex to just add the module and a couple of type extensions
-Renamed the DistributionMonad to RandomVariableMonad

On another note, the FSharp tests are being run as a console application. The ones I've added use NUnit. Are you running the tests in CI? I couldn't find a build script to update it to run the new NUnit tests

@cdrnet
Copy link
Member

cdrnet commented Dec 1, 2012

Great, thanks!

Unit Tests: The main unit tests are fully integrated, but not yet the F# ones. We better convert all of them to NUnit as well, then I'll add another build step for them in TeamCity.

@ovatsus
Copy link
Contributor Author

ovatsus commented Dec 1, 2012

I'm converting the unit tests

@ovatsus
Copy link
Contributor Author

ovatsus commented Dec 1, 2012

Done. Any more feedback?
PS: I plan to do another pull request with the rest of the stuff form PowerPack, but it will probably be in only a few weeks, and it's probably more suitable for a 3.0 version, while this I think can go well in a minor version

@ghost
Copy link

ghost commented Dec 1, 2012

  • Please use postfix type parameters with .NET capitalization and naming

    type RandomVariable<'T>

rather than

type 'a RandomVariable

Idiomatic F# code generally only uses prefix arguments for "list", "option", "[]" and "ref" (if at all).

  • Please also make sure the types are defined in a namespace, rather than in a module so that the user only opens the namespace, i.e.

    namespace MathNet.Numerics

    type Outcome<'T> = ...

    type RandomVariable<'T> = ...

    [<CompilationRepresentation(CompilationRepresentationFlags.ModuleSuffix)>]
    module RandomVariable = ...

  • I think these types and the module should be in a sub-namespace but @cdrnet can advise.

  • Do not define public symbolic operators like =<< and >>= in this library, but define named functions instead.

  • Are the big natural and integer types ever needed? I think not? BigInteger is always available in either System.Numerics or FSharp.Core (if System.Numerics is not present). I think the F# open source build does not yet have a portable library though, so perhaps it is needed for he portable build on Mono, but that should be fixed in the F# open source build.

Thanks
Don

@ghost
Copy link

ghost commented Dec 1, 2012

Also,

@ovatsus
Copy link
Contributor Author

ovatsus commented Dec 1, 2012

Humm, I though the portable version for FSharp.Core didn't have the BigInteger, but while checking again it seems it has, only Parse is missing. I'll revise

@ovatsus
Copy link
Contributor Author

ovatsus commented Dec 1, 2012

Fixed

@ghost
Copy link

ghost commented Dec 2, 2012

Looking at https://github.com/fsharp/fsharp/blob/master/src/fsharp/FSharp.Core/math/z.fsi#L44 I think BigInteger.Parse is there, but not the culture-aware version taking three arguments.

@ovatsus
Copy link
Contributor Author

ovatsus commented Dec 2, 2012

Yes, in the source code it appears so, but in the actual dll (Program Files\Reference Assemblies\Microsoft\FSharp\3.0\Runtime.NETPortable\FSharp.Core.dll) it's not.

@cdrnet cdrnet merged commit d44ac6f into mathnet:master Dec 5, 2012
@cdrnet
Copy link
Member

cdrnet commented Dec 5, 2012

FYI, I've enabled the F# unit tests in TeamCity, works nicely. Thanks again.

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

Successfully merging this pull request may close these issues.

None yet

2 participants