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
prime_powers doesn't work with start very well #13516
Comments
comment:2
as |
This comment has been minimized.
This comment has been minimized.
comment:4
I found the code to be riddled with errors, so I decided to completely rework it. Also, I have qualms about calling 1 a prime power, but did so because the old function did. If you think its fine to drop this, let me know. |
comment:5
Thanks for your work - hopefully someone will review it soon. You can put your real name in the author area.
Well, John Horton Conway does call -1 a prime, in which case every nonzero integer (not just positive) is a unique product of prime powers - not a unique product of primes, note, nor of the exponents, but of the prime powers themselves (I can't find a link for this right now, my apologies) in which case positives get the power 1 and and negatives -1. I think that's right... anyway, maybe they were thinking this? |
comment:6
I would prefer to leave 1 as a prime power because it is listed in Sloane's tables as a prime power: http://oeis.org/A000961 There he says "Since 1 = p^0 does not have a well defined prime base p, it is sometimes not regarded as a prime power.", which might be where your misgivings come from. If by "prime power" one thinks of "power of a prime", the only question is in what set are we considering the prime powers. If we take the natural numbers, then the number 1 is definitely a power of a prime. If by "prime power" one thinks "power of a specific canonical prime", then 1 is not such a thing. In this case, the best thing to do is stick with what is there (to avoid introducing bugs in other people's code!) and clearly document/define what a prime power is in Sage. |
Author: Kevin Halasz |
comment:8
I just updated the docstring to make the fact that 1 is a prime power explicit. |
comment:9
Could you speed this up slightly by making |
comment:10
I've changed the patch so that s=stop.sqrt() is calculated outside of the for loop. After some tests, I saw that this was faster than setting s=stop.sqrt().n(). Also, note that when p>s, the content of that if loop is a break command, meaning that the entire for loop ends. Thus, once a single p>s, no more p values are tried. |
comment:11
The comment on line 708 in I also think that the following:
i.e. the corresponding implementation logic is not right, in the sense that it should just return empty lists rather than throwing exceptions. And negative Then, in the following fragment
|
comment:12
Dimpase, I've addressed all of your suggestions. Let me know what you think of these changes/if you have other suggestions. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
comment:15
You always should coerce
This is because taking Second issue: the comment |
comment:16
In the documentation please write |
comment:17
Dimpase, I'm not sure if I understand what you're suggesting I do. Should I replace the check that the inputs are Integers with a coercion of the inputs into Integers? Once I coerce the elements the check is redundant, as either it raises an error in and of itself (say, if it was passed a string it will raise a TypeError), or will fix the problem. |
comment:18
Replying to @sagetrac-khalasz:
I think I would prefer the input of type |
comment:21
Replying to @sagetrac-khalasz:
I think there is a bug in the code, coming from the fact that
You also should have test cases (doctests) for all the different combinations of start/stop, and test them, too. You know that you can run Sage so that it tests doctests in a particular file, right? |
comment:22
I forgot to rebuild sage before doctesting before posting this patch. Sorry for putting it up with such a stupid mistake. I've fixed it, and added back the test I think I've covered all the possible basic scenarios in the doctests, in both the EXAMPLES and the TESTS, do you disagree? |
comment:24
Some more nitpicks. :)
|
comment:25
Sorry for the delay. I've addressed the small comments. |
comment:26
Thanks a lot for addressing my concerns. I have made some changes to your patch.
The changes can be seen in attachment: 13516_reviewer.patch. All these changes have been merged with your patch and the new patch is now attachment: 13516_primepowers.2.patch. Aside from the above corrections, the changes introduced by your patch has positive review from my side. If you think my changes are ok, feel free to change the ticket to positive review. |
Reviewer: Punarbasu Purkayastha |
This comment has been minimized.
This comment has been minimized.
Changed reviewer from Punarbasu Purkayastha to Dmitrii Pasechnik, Punarbasu Purkayastha |
comment:28
Argggh! Uploaded the wrong patch earlier X( Patchbot: apply 13516_primepowers.2.patch |
This comment has been minimized.
This comment has been minimized.
comment:29
Looks good to me. Thanks for your help/review! |
comment:30
Nice work. Below, things almost certainly not worth addressing now that ppurka and khalasz have worked so hard to get this in, but I'll still point them out if ppurka is really bored and wants to fix them:
|
comment:31
@kcrisman - fixed. Thanks. :) |
comment:32
Ah, I knew I shouldn't have asked about this, because now of course I have to keep following up... you can't do
and have it formatted, because it's in a literal block, it would have to be
or something like that. But I don't know if that's worth it... |
apply only this to devel/sage |
comment:33
Attachment: 13516_primepowers.2.patch.gz Yes, you are right. I have reverted the last change. |
comment:34
Sorry to be so picky, anyway nice work by both of you! |
Changed reviewer from Dmitrii Pasechnik, Punarbasu Purkayastha to Dmitrii Pasechnik, Punarbasu Purkayastha, Karl-Dieter Crisman |
comment:35
No problem. But you do deserve to be in the reviewers list! ;) |
Merged: sage-5.6.beta1 |
See this sage-support thread.
Yeah, this seems problematic. The code in question is old, too, so perhaps there is a more efficient way to do it in the meantime...
Apply to
devel/sage
: attachment: 13516_primepowers.2.patch.CC: @williamstein
Component: number theory
Author: Kevin Halasz
Reviewer: Dmitrii Pasechnik, Punarbasu Purkayastha, Karl-Dieter Crisman
Merged: sage-5.6.beta1
Issue created by migration from https://trac.sagemath.org/ticket/13516
The text was updated successfully, but these errors were encountered: