search

manodeep / Corrfunc

⚡️⚡️⚡️Blazing fast correlation functions on the CPU.
https://corrfunc.readthedocs.io
MIT License
163 stars 50 forks source link

Code quality fixes #189

Closed manodeep closed 4 years ago

pep8speaks commented 5 years ago

Hello @manodeep! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 608:80: E501 line too long (80 > 79 characters) Line 1012:70: E502 the backslash is redundant between brackets Line 1013:69: E502 the backslash is redundant between brackets

Comment last updated at 2019-10-09 21:06:10 UTC
manodeep commented 5 years ago

@lgarrison Please feel free to add any other code quality fixes onto this PR.

manodeep commented 4 years ago

Before merging this PR, we need to add the bug fix for #192 into the CHANGES.rst file.

manodeep commented 4 years ago

@lgarrison When you have a chance, will you give the changes a look-over, particularly the duplicate ngb-cell check that I just added through e53bce3050139337dae5723a3601e707f1f37920.

lgarrison commented 4 years ago

Yes, I'll take a look in more detail soon! But just skimming the diff, it looks like we're doing a brute-force check for cell duplication. Is it not possible to simply construct the neighbor list without the duplicates to begin with?

On Sat, Aug 24, 2019 at 6:02 PM Manodeep Sinha notifications@github.com wrote:

@lgarrison https://github.com/lgarrison When you have a chance, will you give the changes a look-over, particularly the duplicate ngb-cell check that I just added through e53bce3 https://github.com/manodeep/Corrfunc/commit/e53bce3050139337dae5723a3601e707f1f37920 .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/manodeep/Corrfunc/pull/189?email_source=notifications&email_token=ABLA7S63L5CCFH57YY2NATTQGGVXBA5CNFSM4H2VUU42YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5CIIPA#issuecomment-524583996, or mute the thread https://github.com/notifications/unsubscribe-auth/ABLA7S72P7IFWZ3Z3WHL2HDQGGVXBANCNFSM4H2VUU4Q .

manodeep commented 4 years ago

You could almost certainly make the duplication check better. Because that check takes so little time and is likely to come up under very special circumstances, I opted for the brute-force check. Happy to have a brainstorm to see if we can improve the check.

manodeep commented 4 years ago

@lgarrison Will you please take a look at this PR and let me know what you think? We have accumulated a few updates for v2.3.2

manodeep commented 4 years ago

Agreed! We need to either add the threads within that generate_cell_pairs or incorporate the async model to generate and consume the cell-pairs