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

Test for SRTomo class #316

Merged
merged 14 commits into from Sep 28, 2016

Conversation

Projects
None yet
2 participants
@victortxa
Contributor

victortxa commented Sep 9, 2016

Making a test for SRTomo class. Maybe, also changes in docstring of this class to an adjustment.

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)
  • First-time contributor? Add yourself to doc/contributors.rst (leave for last to avoid conflicts)
@leouieda

This comment has been minimized.

Member

leouieda commented Sep 13, 2016

@victortxa thanks for doing this!

To test the predicted method, you can set the p_ attribute by hand to the correct value and compare the result of tomo.predicted() with the forward modeled data.

Also, be careful of PEP8 issues in your code (particularly missing spaces after commas). Use make pep8 to check your code. Travis will catch this as well.

@victortxa

This comment has been minimized.

Contributor

victortxa commented Sep 13, 2016

@leouieda Thank you!
A doubt: p_ is an attribute of SRTomo, so should I call SRTomo.p_=list_of_p_values to set the values ?

@leouieda

This comment has been minimized.

Member

leouieda commented Sep 13, 2016

Something like:

tomo = SRTomo(...)
tomo.p_ = some_array
srcs = [src, src]
recs = [(0, 0), (5, 10)]
ttimes = ttime2d.straight(model, 'vp', srcs, recs)
tomo = srtomo.SRTomo(ttimes, srcs, recs, model)

This comment has been minimized.

@leouieda

leouieda Sep 15, 2016

Member

Please create a mesh object using SquareMesh to use with the tomo class. The model object is mutable and you won't have it if you're not running a synthetic data test.

@@ -0,0 +1,51 @@
import numpy as np

This comment has been minimized.

@leouieda

leouieda Sep 15, 2016

Member

On the first line, use the following imports for future Python 3 compatibility:

from __future__ import division, print_function
from future.builtins import range

This way you don't need to put . in all your numbers "1./2."

@leouieda leouieda added this to the 0.6 milestone Sep 21, 2016

@leouieda

This comment has been minimized.

Member

leouieda commented Sep 21, 2016

@victortxa looks good so far! It seems that line 117 is the one not covered in the tests (it's that check if the predicted is a 1D numpy array). Could you try removing that if statement? If the tests pass and the gallery plot runs as expected then I think it should be safe.

After that, I think all that is left is building the docs and checking is all docstrings and gallery plots are working correctly.

@victortxa

This comment has been minimized.

Contributor

victortxa commented Sep 21, 2016

I'm trying to run tests in my laptop, but I need to fix the installations (a lot of things failing here).

@victortxa

This comment has been minimized.

Contributor

victortxa commented Sep 26, 2016

@leouieda I think it's all working now. I removed the example from the 'docstrings' of the function (take a look if you agree).

@leouieda

This comment has been minimized.

Member

leouieda commented Sep 26, 2016

@victortxa yep, it's all looking good! Thanks!

Could you please merge in the latest commits from master and create the changelog entry? Also, build the docs locally and check that everything is OK, particularly the SRTomo docs and the changelog.

@@ -10,6 +10,8 @@ Version 0.5
**Changes**:
* Add test for SRTomo class.

This comment has been minimized.

@leouieda

leouieda Sep 26, 2016

Member

Please be more detailed, for example:

Implement more unit tests for the ``fatiando.seismic.srtomo`` module. Reached 100% test coverage.

This comment has been minimized.

@victortxa

victortxa Sep 26, 2016

Contributor

Thank you, it was horrible! All seems fine now (docs are build properly, PEP8 is ok too).

@leouieda

This comment has been minimized.

Member

leouieda commented Sep 26, 2016

@victortxa OK, perfect! Sorry, you'll need to merge in master again because #319 was just merged. After that, let me know and I'll merge this.

@victortxa

This comment has been minimized.

Contributor

victortxa commented Sep 28, 2016

@leouieda After checks pass, I think it's done.

@leouieda leouieda merged commit 050fb47 into fatiando:master Sep 28, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 73.442%
Details
@leouieda

This comment has been minimized.

Member

leouieda commented Sep 28, 2016

Merged!

@leouieda leouieda modified the milestones: 0.6, 0.5 Sep 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment