manodeep / Corrfunc

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

Docs #205

Closed manodeep closed 4 years ago

manodeep commented 4 years ago

Fixes #203 and a niggling issue with the docs.

Thanks @1313e for the help!

manodeep commented 4 years ago

@lgarrison Currently, we are asking people to cite the ASCL entry for Corrfunc v2.0.0, and then the proceedings in addition if they are using >= v2.3.0. Are you okay with continuing that? Other options include asking people to cite the MNRAS paper for v2.0.0

Related: we need to come up with an easy way to display the citation info from within the code-base (populated automatically based on the code version)

lgarrison commented 4 years ago

Maybe we should ask people to cite the MNRAS paper, since that's referred? It would be great if people cited the proceedings paper too, but maybe that's overkill for ordinary users (as opposed to developers specifically interested in the high-performance aspects).

On Wed, Dec 18, 2019, 12:18 AM Manodeep Sinha notifications@github.com wrote:

@lgarrison https://github.com/lgarrison Currently, we are asking people to cite the ASCL entry for Corrfunc v2.0.0, and then the proceedings in addition if they are using >= v2.3.0. Are you okay with continuing that? Other options include asking people to cite the MNRAS paper for v2.0.0

Related: we need to come up with an easy way to display the citation info from within the code-base (populated automatically based on the code version)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/manodeep/Corrfunc/pull/205?email_source=notifications&email_token=ABLA7S5522XTODAXKTB3S63QZGXD7A5CNFSM4J3UXPFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHE4OVA#issuecomment-566871892, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABLA7SZRBHCIX53TIAGGI6LQZGXD7ANCNFSM4J3UXPFA .

manodeep commented 4 years ago

I am conflicted here - asking people to continue citing the ASCL entry means all the citations are reflected by one entry. It has already been time-consuming to track the citations to the Zenodo DOI and then submitting reference corrections to ADS about switching those to the ASCL one. If we ask people to cite the MNRAS paper, then showing the impact will become a little more involved. For instance, here is the citation history for the ASCL entry:


Screen Shot 2019-10-11 at 12 51 30 pm

Showing such a plot with two bib-entries will be a little more involved. That said, I do not feel strongly about this - @lgarrison if you have strong feelings that new citations should go to the MNRAS paper, then I am happy to go with that.

Regarding the conference proceedings, I like the idea of only asking power users to cite that paper. If I understand correctly, you are suggesting that HPC implementations that borrow/modify/heavily use the Corrfunc kernels would cite that paper.

lgarrison commented 4 years ago

I think it doesn't much matter which we ask people to cite, because the citations are going to end up split no matter what we do! Both ASCL and MNRAS will show up in ADS searches, so some will pick one and some the other. I don't feel strongly either, but it's kind of nice that we finally wrote a real code paper about Corrfunc, so maybe it's worth asking people to start citing that.

That's basically was I was thinking re: the AVX512. If the AVX512 is particularly enabling for a user, a citation might be appropriate.

On Wed, Dec 18, 2019 at 2:43 PM Manodeep Sinha notifications@github.com wrote:

I am conflicted here - asking people to continue citing the ASCL entry means all the citations are reflected by one entry. It has already been time-consuming to track the citations to the Zenodo DOI and then submitting reference corrections to ADS about switching those to the ASCL one. If we ask people to cite the MNRAS paper, then showing the impact will become a little more involved. For instance, here is the citation history for the ASCL entry:

[image: Screen Shot 2019-10-11 at 12 51 30 pm] https://user-images.githubusercontent.com/3004941/71117307-01a85000-222a-11ea-9d6b-b8c2388c3e33.png

Showing such a plot with two bib-entries will be a little more involved. That said, I do not feel strongly about this - @lgarrison https://github.com/lgarrison if you have strong feelings that new citations should go to the MNRAS paper, then I am happy to go with that.

Regarding the conference proceedings, I like the idea of only asking power users to cite that paper. If I understand correctly, you are suggesting that HPC implementations that borrow/modify/heavily use the Corrfunc kernels would cite that paper.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/manodeep/Corrfunc/pull/205?email_source=notifications&email_token=ABLA7S4FEHUIT7FWMQTUUT3QZJ4NBA5CNFSM4J3UXPFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHHIGMQ#issuecomment-567182130, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABLA7S26N3RE3RZPCOJ6ZGDQZJ4NBANCNFSM4J3UXPFA .

-- Lehman Garrison lgarrison@flatironinstitute.org Flatiron Research Fellow, Cosmology X Data Science Group Center for Computational Astrophysics, Flatiron Institute lgarrison.github.io

manodeep commented 4 years ago

@lgarrison I have updated the citation recommendation in both the README and the docs. In the latest commit, I took at stab at fixing #195

Will you please take a look at this PR when you get a chance.

lgarrison commented 4 years ago

Makes sense, thanks!

On Mon, Dec 23, 2019, 3:00 PM Manodeep Sinha notifications@github.com wrote:

@manodeep commented on this pull request.

In docs/source/apidoc.sh https://github.com/manodeep/Corrfunc/pull/205#discussion_r360989145:

if ! python -c 'import numpydoc'; then easy_install --user numpydoc; fi if ! python -c 'import sphinx'; then easy_install --user sphinx; fi

-sphinx-apidoc -H "Comprehensive API reference" -M -f -o source/api/ ../Corrfunc/ +outdir=source/api +sphinx-apidoc -H "Comprehensive API reference" -M -f -o $outdir ../Corrfunc/ ../Corrfunc/tests.py ../Corrfunc/call_correlation_functions.py ../Corrfunc/call_correlation_functions_mocks.py + +# Fix the blank sub-modules in the Corrfunc file +for docfile in $outdir/Corrfunc.rst +do

  • Delete three lines following the "submodules"

  • sed -e '/Submodules/{N;N;d;}' $docfile > xx

That works on linux but not on OSX, and the one that works on oSX does not work on linux. That's why I chose this two-step process that works on both.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/manodeep/Corrfunc/pull/205?email_source=notifications&email_token=ABLA7S3LAKACHV4BLB4TUYTQ2EKFPA5CNFSM4J3UXPFKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCQDNYPQ#discussion_r360989145, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABLA7S6BBFPQOJQ5IZL6I7DQ2EKFPANCNFSM4J3UXPFA .

manodeep commented 4 years ago

@lgarrison Instead of hard-coding the name for the temporary file, the name is now auto-generated. Also updated to the proper handling of filenames within shell scripts.

I am going to merge this PR in and make the release for 2.3.2 (my) today - is that fine?

lgarrison commented 4 years ago

Yes, all fine! Looks ready.