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.

Sign up for GitHub

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

Jump to bottom

add prodfactors and radical functions #42

Merged
merged 1 commit into from
May 19, 2017
Merged

add prodfactors and radical functions #42

merged 1 commit into from
May 19, 2017

Conversation

rfourquet
Copy link
Member

@rfourquet rfourquet commented Apr 28, 2017 •
edited
Loading

EDIT TITLE: renamed factorback -> prodfactors (not updated below).
Add factorback such that factorback(Container, factor(n)) == n (cf. docstrings for details).
The name ("factorback" comes from the "pari" package) can of course be bikeshed, cf. JuliaLang/LinearAlgebra.jl#229.
I like also the name factorprod (but there was an objection in the linked issue).
I also added the function radical as it was natural to do so, but didn't export it (yet).

ararslan
ararslan reviewed Apr 28, 2017
View reviewed changes
src/Primes.jl Outdated
"""
function factorback end

factorback(factors::Associative) = prod(p^i for (p, i) in factors)
Copy link
Member

Choose a reason for hiding this comment

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

Julia 0.4 has array comprehensions but not Generators, which is causing the test failures on 0.4. I think we should just drop 0.4 support at this point. I can submit a PR for that later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I keep forgetting about that!

@ararslan
Copy link
Member

I also added the function radical as it was natural to do so, but didn't export it (yet).

Might as well export it, IMO, or at least add it to the docs. Seems pretty cool.

@ararslan
Copy link
Member

I'm not 100% sold on the name factorback, as I don't find it immediately descriptive, but I'm also not really opposed to it. A term that comes to mind is "expand," as the inverse of factorization is expansion when talking about polynomials, but I don't know whether that term makes sense for integer factorizations.

@rfourquet
Copy link
Member Author

I actually also like expand, and thought about it but was drawn away by the fact that it has a totally different meaning in Base. But it is also a shame that this name (with mathematical meaning) would be reserved for a technical functionality of julia code. Considering opening a Julia issue for freeing that name...

@rfourquet
Copy link
Member Author

Ok, exported radical, removed the generator expression and added a test in the hope to have coveralls happier.

@ararslan
Copy link
Member

Even if we could reclaim expand from Base, it would be a while before we could do so. First it would have to be deprecated there, and it's too late to add new deprecations to 0.6, so it would have to be deprecated in the next release and removed after. So the soonest we could use it would be 1.0, and we still wouldn't be able to use if then if we wanted to maintain support for older Julia versions.

Now, if we wanted to support only going from a Primes.Factorization to an integer, we could overload Base.expand for that, since our factorization type is not in Base and expand currently only takes an Expr as an argument. But it's type piracy if we wanted to use anything more general for reconstructing the integer, e.g. any Associative.

We could do something like expandfactors. It's a little long, but it's descriptive.

ararslan
ararslan approved these changes Apr 29, 2017
View reviewed changes
Copy link
Member

@ararslan ararslan left a comment

Choose a reason for hiding this comment

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

Looks good to me, pending any renaming of factorback.

ararslan
ararslan reviewed Apr 30, 2017
View reviewed changes
src/Primes.jl Outdated
"""
function factorback end

factorback(factors::Associative) = prod([p^i for (p, i) in factors])
Copy link
Member

Choose a reason for hiding this comment

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

Support for 0.4 is now a thing of the past, so this can be the bare generator expression again if you'd prefer. I did a benchmark to see whether having it in an array comprehension versus a generator made a difference and it doesn't for the most part, so changing isn't really necessary, just thought I'd give you a heads up.

ararslan
ararslan reviewed Apr 30, 2017
View reviewed changes
docs/src/api.md Outdated
@@ -4,6 +4,7 @@

```@docs
Primes.factor
Primes.factorback
Copy link
Member

Choose a reason for hiding this comment

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

Could you add radical here too? (Or wherever the appropriate place in the docs is)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch.

@rfourquet
Copy link
Member Author

rfourquet commented Apr 30, 2017 •
edited
Loading

Indeed Base.expand would be type piracy for anything other than Primes.Factorization, for which I already proposed prod here. I realize that indeed for a general Associative, the name should contain factor. As prod seems good (to me) for Factorization, I find factor(s)prod or prodfactors more consistent. Though expandfactors sounds good too. Will keep open few more days to allow more people to comment on a prefered name.

@rfourquet
Copy link
Member Author

Someone likes to feed the bikeshed here?!

@rfourquet
Copy link
Member Author

Instead of factorback, what about having a non-exported Primes.prod? Base.prod would work for Primes.factorization, and Primes.prod would have to be used for other containers...

@ararslan
Copy link
Member

Hm, I'm not really a fan of shadowing Base functions like that. I do still like expandfactors but if you're not into that, I think factorback is fine.

@rfourquet
Copy link
Member Author

Let's forget about the shadowing (of which I'm not a big fan either), I prefer expandfactors to factorback, but I think I prefer even more prodfactors because prod is already used for Factorization. Opinions @pabloferz @simonbyrne ?

@ararslan
Copy link
Member

Ah yeah, I like prodfactors.

@rfourquet rfourquet changed the title add factorback and radical functions add prodfactors and radical functions May 19, 2017
@rfourquet rfourquet merged commit e7021b6 into master May 19, 2017
@rfourquet rfourquet deleted the rf/factorback branch May 19, 2017 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ararslan ararslan ararslan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rfourquet @ararslan