Skip to content
This repository was archived by the owner on Dec 22, 2021. It is now read-only.

Enable duecredit for magdir#293

Merged
leouieda merged 5 commits into
fatiando:masterfrom
mtb-za:duecredit_testing
Jul 13, 2016
Merged

Enable duecredit for magdir#293
leouieda merged 5 commits into
fatiando:masterfrom
mtb-za:duecredit_testing

Conversation

@mtb-za

@mtb-za mtb-za commented Jul 8, 2016

Copy link
Copy Markdown
Contributor

Fixes #222 and test for general rollout of #292

Adds the ability to easily get the citation of a function that was used.

To test:

  1. Install duecredit: pip install duecredit

(This should be doable without installing it globally, but that is not working
for me. I may be missing something here with importing the module, because
this is what our_duecredit.py is supposed to enable, if I understand their
docs correctly.)

Then, either:

  1. Call the script with python -m duecredit gravmag_magdir_dipolemagdir.py.

OR

  1. Set an environment variable: DUECREDIT_ENABLE=yes.
  2. Run python cookbook\gravmag_magdir_dipolemagdir.py.

Any output from the script will be shown first, followed by something like:

DueCredit Report:
- Estimates total mag. dir. of approx. spherical bodies /
fatiando.gravmag.magdir:DipoleMagDir (v 0.4-21-gcb9ce97-dirty) [1]
- Scientific tools library / numpy (v 1.10.4) [2]

1 package cited
0 modules cited
1 function cited

References
----------

[1] Oliveira, V.C. et al., 2015. Estimation of the total magnetization direction
of approximately spherical bodies. Nonlin. Processes Geophys., 22(2),
pp.215-232.

Checklist:

  • Make tests for new code (at least 80% coverage)
  • Create/update docstrings
  • Include relevant equations and citations in docstrings
  • Docstrings follow the style conventions
  • Code follows PEP8 style conventions
  • Code and docs have been spellchecked
  • Include new dependencies in doc/install.rst, requirements.txt, environment.yml, ci/requirements-conda.txt and ci/requirements-pip.txt.
  • Documentation builds properly (run cd doc; make locally)
  • Changelog entry (leave for last to avoid conflicts)

Closes fatiando#222

Adds the ability to easily get the citation of a function that was used.

To test:
1. Install duecredit: `pip install duecredit`

Then, either:
1. Call the script with `python -m duecredit
gravmag_magdir_dipolemagdir.py`.

OR

1. Set an environment variable: `DUECREDIT_ENABLE=yes`.
2. Run `python cookbook\gravmag_magdir_dipolemagdir.py`.

Either method should output something like the following on the command
line.
Any output from the script will be shown first.
===========
DueCredit Report:
- Estimates total mag. dir. of approx. spherical bodies /
fatiando.gravmag.magdir:DipoleMagDir (v 0.4-21-gcb9ce97-dirty) [1]
- Scientific tools library / numpy (v 1.10.4) [2]

1 package cited
0 modules cited
1 function cited

References
----------

[1] Oliveira, V.C. et al., 2015. Estimation of the total magnetization
direction
of approximately spherical bodies. Nonlin. Processes Geophys., 22(2),
pp.215-232.
@leouieda leouieda added the docs label Jul 8, 2016
@leouieda leouieda added this to the 0.5 milestone Jul 8, 2016
@leouieda

leouieda commented Jul 8, 2016

Copy link
Copy Markdown
Member

@mtb-za wow thanks for this! That was very quick.

From what I understood of their docs, you can only get the citation information if you have duecredit installed. The stub that we commit to our repository only does something if it can import duecredit [1]. If not, then it does nothing and doesn't bother us. That is a good thing because it doesn't add yet another dependency for us.

@leouieda

leouieda commented Jul 8, 2016

Copy link
Copy Markdown
Member

While you're at it, could you also please add the following to the docstring of DipoleMagDir?

After the first line of the docstring add the line:

Implements the method of Oliveira Jr. et al. (2015).

and place the reference after Blakely:

Oliveira Jr., V. C., D. P. Sales, V. C. F. Barbosa, and L. Uieda (2015), 
Estimation of the total magnetization direction of approximately spherical bodies, 
Nonlin. Processes Geophys., 22(2), 215–232, doi:10.5194/npg-22-215-2015.

That way we are complete with #222

@leouieda

leouieda commented Jul 8, 2016

Copy link
Copy Markdown
Member

One more thing, maybe we should rename our_duecredit.py to _our_duecredit.py so that it is hidden from users (this is only meant to be used internally). You can rename in git with git mv.

mtb-za added 2 commits July 11, 2016 11:18
Added reference to the docstring as usual.

Renamed duecredit's stub to be internal for us, not public facing.
Forgot to change the import after moving the duecredit stub.
@mtb-za

mtb-za commented Jul 12, 2016

Copy link
Copy Markdown
Contributor Author

This should be pretty much done now. If someone gives it the OK, I will edit the changelog and it can be merged.

@leouieda

Copy link
Copy Markdown
Member

This is perfect, thanks Martin! You can add the changelog entry (please provide a link to the duecredit.org website there). Also, make sure the documentation builds properly after the change. Let me know when it's done and I'll merge this.

@mtb-za

mtb-za commented Jul 13, 2016

Copy link
Copy Markdown
Contributor Author

I am having trouble running make on this computer. The last commit fixed an error in the Travis build for the docs, so I think it is working now.

I will try and check this in the next couple days on a *nix machine. But if someone else can check before then, that would be good.

The output for the docs from Travis is here:

building [mo]: targets for 0 po files that are out of date
building [html]: targets for 122 source files that are out of date
updating environment: 135 added, 0 changed, 0 removed
reading sources... [100%] news                                                  
looking for now-outdated files... none found
pickling environment... done
checking consistency... done
preparing documents... done
writing output... [100%] news                                                   
generating indices... genindex py-modindex
highlighting module code... [100%] fatiando.gravmag.basin2d                     
writing additional pages... search
copying images... [100%] gallery/5-seismic/images/thumb/sphx_glr_convolutional_model_thumb.png
copying downloadable files... [100%] _static/cookbook/mesher_prismmesh.py       
copying static files... WARNING: favicon file u'favicon.ico' does not exist
done
copying extra files... done
dumping search index in English (code: en) ... done
dumping object inventory... done
build succeeded, 1 warning.
Embedding documentation hyperlinks in examples..
    processing: index.html
    processing: point_scatter.html
    processing: regular_grid.html
    processing: mayavi-exaggerate.html
    processing: seismic-wiggle.html
    processing: prism_mesh.html
    processing: prism_model.html
    processing: eqlayer_mag_transform.html
    processing: eqlayer_transform.html
    processing: euler_expanding_window.html
    processing: euler_moving_window.html
    processing: convolutional_model.html
    processing: srtomo_regularized.html
[done]

Build finished. The HTML pages are in _build/html.
make: Leaving directory `/home/travis/build/fatiando/fatiando/doc'
Finished building documentation

Comment thread doc/changelog.rst Outdated
to subsections.
(`PR 275 <https://github.com/fatiando/fatiando/pull/275>`__)
* Added back-end support for decorators from duecredit
(`<https://github.com/duecredit/duecredit/>`__) to be added to methods. This

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please replace

duecredit  (`<https://github.com/duecredit/duecredit/>`__)` 

with

`duecredit  <https://github.com/duecredit/duecredit/>`__

The syntax

text <target>__

in rst is equivalent to

[Text](target)

In Markdown. I get this wrong all the time. That's one of the reasons i added "build the docs locally" to the check list 😄

@leouieda

Copy link
Copy Markdown
Member

Are you getting an error running make or don't have it installed?

Unicode support is one the reasons I really want to move to Python 3. That - in the page numbers of the reference always gets me.

@mtb-za

mtb-za commented Jul 13, 2016

Copy link
Copy Markdown
Contributor Author

It is installed, but is giving me an error when I run make all:

sh: C:\Program: No such file or directory
Makefile:30: recipe for target 'cookbook' failed
make: *** [cookbook] Error 127

This seems to be something related to a space in the path to sh, since the one it is using is C:\Program Files (x86)\Git\bin\sh.exe. cf: http://stackoverflow.com/questions/5999507/mingw-make-cant-handle-spaces-in-path
I will run it locally on a *nix machine tomorrow though, which should not have this issue. :)

Having trouble running make on this computer: it is complaining about
the path to `sh`.

The travis build suggests that there is a problem here though:
building [html]: targets for 122 source files that are out of date
updating environment: 135 added, 0 changed, 0 removed
reading sources... [  9%] api/gravmag.magdir

Encoding error:
'ascii' codec can't decode byte 0xe2 in position 2846: ordinal not in
range(128)
The full traceback has been saved in /tmp/sphinx-err-5FAnuX.log, if you
want to report the issue to the developers.
make: *** [html] Error 1
make: Leaving directory `/home/travis/build/fatiando/fatiando/doc'
Finished building documentation

This commit should remove the only non-ASCII character that I
introduced.

Fixed syntax in changelog.rst file
@mtb-za mtb-za force-pushed the duecredit_testing branch from 036c028 to 1bb59fe Compare July 13, 2016 15:09
@leouieda

Copy link
Copy Markdown
Member

@mtb-za using command-line tools in Windows is always a pain. OK, I'll run it here as well and see if I catch anything.

@leouieda

Copy link
Copy Markdown
Member

@mtb-za Travis seems to be OK and the docs build perfectly on my machine, so I'll merge this now.

Thanks again for doing this so quickly!

@leouieda leouieda merged commit a57522f into fatiando:master Jul 13, 2016
@mtb-za mtb-za deleted the duecredit_testing branch July 14, 2016 08:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants