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

Add copy method to mesher classes #301

Merged
merged 13 commits into from Aug 31, 2016

Conversation

Projects
None yet
2 participants
@PiotrKurnik
Member

PiotrKurnik commented Jul 17, 2016

Fixes #284

I added copy method to Square. Also I wrote some tests. This is for a try, if OK I will make further changes.

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

This comment has been minimized.

Member

leouieda commented Jul 18, 2016

Thank you for doing this! The code and tests are looking pretty good! I don't think there is much else to do but a few minor changes and documentation.

The reason Travis CI is failing is because of the style check (PEP8). See here for the error log. You just have one too many lines after the copy method. You can run make pep8 locally to check this.

One other thing I would ask is that you rename your test file to test_square.py because it will contain only the tests for Square.

def test_square_copy():
orig = Square([0, 1, 2, 4], {'density': 750})
cp = orig.copy()

This comment has been minimized.

@leouieda

leouieda Jul 18, 2016

Member

Another check you do is to check is an object is the same reference as another using the is comparison: cp is not orig.

Two objects might be equal but not be the same object.

@leouieda leouieda added this to the 0.5 milestone Jul 18, 2016

@leouieda

This comment has been minimized.

Member

leouieda commented Jul 21, 2016

@PiotrKurnik let me know when you make some of these changes so that I can take a look. No pressure and no hurry 😄

One things I forgot to mention is that the copy method needs a docstring. Please try to follow the style of the others. See http://www.fatiando.org/dev/develop.html#documentation for a specification.

@PiotrKurnik

This comment has been minimized.

Member

PiotrKurnik commented Jul 27, 2016

@leouieda I have added copy to other classes and I have written a few more tests. It's not finished yet, just wanted to see if Travis will pass the tests. It shouldn't take long to complete this.

@leouieda

This comment has been minimized.

Member

leouieda commented Jul 27, 2016

@PiotrKurnik OK! Let me know when you're finished and I'll review the code to see if there is anything else needed.

Great job on this!

def test_prism_mesh_copy():
p1 = PrismMesh((0, 1, 0, 2, 0, 3), (1, 2, 2))
p1.addprop('density', 3200)

This comment has been minimized.

@leouieda

leouieda Jul 27, 2016

Member

For the mesh classes in Fatiando, the physical properties are arrays of values (one for each cell in the mesh). Could you test this using arrays? You can create the array with something like p1.addprop('density', 3200 + np.zeros(p1.size))

@leouieda

This comment has been minimized.

Member

leouieda commented Aug 4, 2016

@PiotrKurnik one thing I just remembered, all basic geometric types (Square, Prism etc but not PrismMesh etc) inherit from GeometricElemennt. So you can implement the copy method there and they will all get it through inheritance. Sorry it took me so long to spot this. I've haven't looked at the mesher code for quite some time.

@PiotrKurnik

This comment has been minimized.

Member

PiotrKurnik commented Aug 4, 2016

@leouieda Good spot! I have pushed the code. Let's see if it's any closer now.

@leouieda leouieda modified the milestones: 0.5, 0.6 Aug 23, 2016

def copy(self):
""" Return a deep copy of the current instance."""
return cp.deepcopy(self)

This comment has been minimized.

@leouieda

leouieda Aug 29, 2016

Member

You can remove this one since Polygon will inherit it from GeometricElement.

@@ -9,11 +9,12 @@ Version 0.5
**Release date**: yyyy-mm-dd
**Changes**:
* Add copy method to fatiando.mesher.

This comment has been minimized.

@leouieda

leouieda Aug 29, 2016

Member

Could you please change this to the following?

Add a copy method to ``fatiando.mesher`` objects.
@@ -9,11 +9,12 @@ Version 0.5
**Release date**: yyyy-mm-dd
**Changes**:
* Add copy method to fatiando.mesher.
('PR 301 <https://github.com/fatiando/fatiando/pull/301>`__)

This comment has been minimized.

@leouieda

leouieda Aug 29, 2016

Member

The first ' should be a back tic like the closing one.

@leouieda

This comment has been minimized.

Member

leouieda commented Aug 29, 2016

@PiotrKurnik all looks good! Just made a few minor comments. Once those are done let me know and I'll hapilly merge this. Thank you for putting time into this!

@PiotrKurnik

This comment has been minimized.

Member

PiotrKurnik commented Aug 29, 2016

Thanks @leouieda. I am sorry it took so long.

@@ -14,6 +14,7 @@ A (hopefully) updated list of people who contributed to Fatiando a Terra:
* `Martin Bentley`_ - Nelson Mandela Metropolitan University, South Africa and AEON, South Africa
* `Victor Almeida`_ - UERJ, Brazil.
* `M. Andy Kass`_ - United States Geological Survey, USA
* `Piotr Kurnik`_ - United Kingdom

This comment has been minimized.

@leouieda

leouieda Aug 29, 2016

Member

Do you want to link your name to some page (a personal page, github profile, etc)? If so, add the link to the lines below like the other contributors. If not, you can remove the back tics and the underline.

Sorry that I didn't see this before.

This comment has been minimized.

@PiotrKurnik

PiotrKurnik Aug 30, 2016

Member

I removed the back ticks.

@leouieda

This comment has been minimized.

Member

leouieda commented Aug 29, 2016

@PiotrKurnik no problem! We're all busy with other work and I'm the last one that can complain.

* Move from ``distutils`` to ``setuptools`` in ``setup.py``, as recommended in
the `Python Packaging User Guide <https://packaging.python.org/>`__.
(`PR 294 <https://github.com/fatiando/fatiando/pull/294>`__)
* Enable ``fatiando.mesher.PointGrid`` to have points at different depths by
* Enable ``fatiando.mesher.PointGrid`a` to have points at different depths by

This comment has been minimized.

@leouieda

leouieda Aug 31, 2016

Member

Looks like an a was inserted here by mistake. Could you please delete it?

This comment has been minimized.

@PiotrKurnik

PiotrKurnik Aug 31, 2016

Member

no idea where it came from ....

@leouieda

This comment has been minimized.

Member

leouieda commented Aug 31, 2016

@PiotrKurnik everything builds properly on my machine and we're ready to merge!

@leouieda leouieda modified the milestones: 0.5, 0.6 Aug 31, 2016

@leouieda leouieda changed the title from Copy of square to Add copy method to mesher classes Aug 31, 2016

@leouieda leouieda merged commit 84d511e into fatiando:master Aug 31, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.7%) to 73.31%
Details
@leouieda

This comment has been minimized.

Member

leouieda commented Aug 31, 2016

@PiotrKurnik merged! Congratulations on your first contribution (of many I hope)!

@leouieda leouieda referenced this pull request Sep 13, 2016

Closed

Mesher package #315

0 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment