Feat/refine tree #141
Feat/refine tree #141
Conversation
fourndo
commented
Mar 19, 2019
•
fourndo
commented
- Add functionality for TreeMesh creation
- General bug fix on refine function
There was a problem hiding this comment.
I just left a few comments with regards to meshBuilderXYZ, as I looked at it with regards to #144 .
I think the comments I made is along discussions I had in another thread with @lheagy. PeP8-conventions that were not followed in SimPEG originally, but should be followed for new code.
As I am not familiar with the TreeMesh implementiation I won't do a full review.
| 'are implemented') | ||
|
|
||
| assert verticalAlignment in ['center', 'top'], ("verticalAlignment must be 'center' | 'top'") | ||
|
|
There was a problem hiding this comment.
Replace assert with an if-statement and raise. assert should only be used for testing with unittest/pytest in test_xyz.py-files, as certain flags will remove or ignore assert statements.
| expFact=1.3, | ||
| meshType='TENSOR', | ||
| verticalAlignment='top' | ||
| ): |
There was a problem hiding this comment.
The naming convention states CapsWords for classes, but lower_case_with_underscores for functions and variables (https://www.python.org/dev/peps/pep-0008/#function-and-variable-names). Even though SimPEG is not strict in this, I think it would be preferred that new code follows it. Hence def mesh_builder_xyz, and pad_x, exp_fact, mesh_type, vertical_alignment etc.
discretize/utils/meshutils.py
Outdated
| return nC | ||
|
|
||
| # Figure number of padding cells required to fill the space | ||
| npadEast = expand(h[0], padX[0]) |
There was a problem hiding this comment.
Naming again here and in many other places, e.g., npad_east, pad_x, exp_fact, ....
Codecov Report
@@ Coverage Diff @@
## master #141 +/- ##
==========================================
- Coverage 82.74% 82.26% -0.49%
==========================================
Files 22 22
Lines 4556 4725 +169
==========================================
+ Hits 3770 3887 +117
- Misses 786 838 +52
Continue to review full report at Codecov.
|
…into feat/refineTree
|
@lheagy Can you give me a hand with this? Can't figure out where the error come from, and the plot doesn't seem to show... |
|
@dom, for sure. Can you give me a bit more context? is this plot in the test? |
|
Plot is in |
discretize/utils/meshutils.py
Outdated
| mesh_type='TENSOR' | ||
| ): | ||
| """ | ||
| Function to quickly generate a Tensor of Tree mesh |
discretize/utils/meshutils.py
Outdated
|
|
||
| """ | ||
| if mesh_type not in ['TENSOR', 'TREE']: | ||
| raise Exception( |
There was a problem hiding this comment.
I think raising a ValueError is the more appropriate exception here, instead of just throwing a general exception.
discretize/utils/meshutils.py
Outdated
| if octree_levels_padding is not None: | ||
|
|
||
| if len(octree_levels_padding) != len(octree_levels): | ||
| raise Exception( |
There was a problem hiding this comment.
Again ValueError is probably more appropriate.
discretize/utils/meshutils.py
Outdated
|
|
||
| return mesh.max_level-ii | ||
|
|
||
| return 0 |
There was a problem hiding this comment.
could it be good to add a min_level parameter? so that you'll always get cells at least at that level? Maybe something that gets called first before any of these three get evaluated? A simple mesh.refine(min_level) should work.
…into feat/refineTree
|
@jcapriot Just need to add an example, but all systems green on my end. |
There was a problem hiding this comment.
This is looking good! Sorry it took me so long to take a look. I made a few minor suggestions. When I built the docs locally, the image showed up, so I am not entirely sure where the problem is happening on the windows side @fourndo - we can follow up in slack.
Co-Authored-By: fourndo <fourndo@gmail.com>
Co-Authored-By: fourndo <fourndo@gmail.com>
Co-Authored-By: fourndo <fourndo@gmail.com>
Co-Authored-By: fourndo <fourndo@gmail.com>
Co-Authored-By: fourndo <fourndo@gmail.com>
|
@lheagy We get a little drop in coverage because of the plotting function, but good to go whenever you are. |
Adding in some error testing.
There was a problem hiding this comment.
This looks good! Just one request - can we move the docs over to numpy-style?
discretize/utils/meshutils.py
Outdated
| on the underlaying mesh to reduce interpolation errors. | ||
| The core extent is set by the bounding box of the xyz location. | ||
|
|
||
| :param numpy.ndarray xyz: Location points [n x dim] |
There was a problem hiding this comment.
For future docs, can we stick with the numpy-style?
If it is helpful, I can reformat this and push it back, just let me know :)
