Optionally keep guard cells #30

TomNicholas · 2019-03-20T17:12:09Z

This adds options keep_xguards and keep_yguards to open_boutdataset, which will stop the preprocessing function from trimming the cells at the edges of the domain. Addresses #19

It works by inferring the location of the dump file within the whole domain using the number of the dump file and know of the overall processor splitting in each direction.

This isn't finished though - some of the tests need to be rewritten as their use of fixtures is pretty dodgy (I think there was a better way to do them with pytest that I didn't know about when I wrote them, but I'll have a look and push to this branch).

It also assumes there are only boundary cells at the outer edges of the domain, so I think it might not yet work with geometries which have a private flux region? This might break my nice logic in _infer_contains_guards()...

(Note to self: this works with the master branch of xarray rather than with my PR)

johnomotani · 2019-03-20T17:17:37Z

it might not yet work with geometries which have a private flux region?

Should be fine for single-null geometries. Double-null is trickier because the second pair of divertor targets (y-boundries) is in the middle of the grid. I've got a PR coming soon for BOUT-dev to load those y-boundary cells from grid files, which shows the logic needed. It would probably be easier to implement here if we saved global x,y,z indices in the dump files...

TomNicholas · 2019-03-20T17:29:48Z

Should be fine for single-null geometries. Double-null is trickier

Ah yes I meant double-null geometries.

It would probably be easier to implement here if we saved global x,y,z indices in the dump files...

That would make it easier but would also be a breaking change... Relates to #3
I don't think we have to have global indices to do this, as long as the position of the second pair of targets can be deduced from the variables in a single dump file.

codecov-io · 2019-03-20T17:38:05Z

Codecov Report

Merging #30 into master will increase coverage by 3.59%.
The diff coverage is 87.09%.

@@            Coverage Diff             @@
##           master      #30      +/-   ##
==========================================
+ Coverage   48.03%   51.62%   +3.59%     
==========================================
  Files           6        6              
  Lines         254      277      +23     
  Branches       51       56       +5     
==========================================
+ Hits          122      143      +21     
- Misses        122      123       +1     
- Partials       10       11       +1

Impacted Files	Coverage Δ
xbout/boutdataset.py	`37.07% <100%> (ø)`	⬆️
xbout/load.py	`86.91% <86.66%> (+1.2%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc503e7...82ff092. Read the comment docs.

ZedThree · 2019-03-22T13:05:59Z

xbout/load.py

@@ -151,31 +155,82 @@ def _arrange_for_concatenation(filepaths, nxpe=1, nype=1):
    return paths_grid, concat_dims


-def _trim(ds, ghosts={}, keep_guards=True):
+def _trim(ds, ghosts, guards={}, keep_guards={}, nxpe=1, nype=1):


=None rather than ={} as a default argument. This does mean you then need to check if it's None, and if so set a default

ZedThree · 2019-07-02T14:18:42Z

xbout/load.py

+                lower = None
+            else:
+                lower = max(ghosts[dim], guards[dim])
+        elif ghosts.get(dim, False):


ghosts[dim] is optional here, but required above (max(ghosts[dim], guards[dim])

Might just need documenting?

Technically this is fine because you would never have a dimension which has guard cells but no ghost cells, but it should be made consistent for clarity and ease of testing.

ZedThree · 2019-07-02T14:21:05Z

xbout/load.py

+        elif ghosts.get(dim, False):
+            upper = -ghosts[dim]
+        else:
+            upper = None


The logic for upper is almost the same as for lower -- is it possible to pull out into a helper function?

Probably - I'll have a go

As they are not completely identical, it might be more trouble that its worth

I managed to factor this out, so do you think this is ready to merge now @ZedThree ?

ZedThree · 2019-07-02T14:23:43Z

What are the differences between "ghost" and "guard" cells?

TomNicholas · 2019-07-02T14:30:19Z

I was using the definitions:

Ghost cells - internal boundary cells around a processor which are not at the edge of the domain, only used for communication, duplicated between processors.

Guard cells - external boundary cells around a processor which are at the edge of the domain. Might be physically-meaningful for certain bcs, for example Debye sheath.

So you might want to keep guard cells in your output but you'll never want to keep ghost cells.

ZedThree · 2019-07-02T14:33:51Z

Ah, ok. In BOUT++ we usually use "guard cells" to mean what you call "ghost cells", and "boundaries" to mean "guard cells", although not always consistently, unfortunately. We also always have the same number of guard cells as boundary cells.

We may need to be a bit careful here.

TomNicholas · 2019-07-02T14:40:57Z

Okay, sounds like I should change the names here to match BOUT conventions (even if they're not rigorously adhered to). I'll also add a clear explanation of the difference to the docs somewhere (probably just the docstring of the functions which have keep_xguards etc as arguments).

bendudson · 2019-07-02T14:44:53Z

Thanks @TomNicholas and @ZedThree . I think if this naming is documented somewhere then we can try to make it consistent across the code and documentation.

_trim was missing a required argument.

Since _trim is a private method, and in the only place it is used all the arguments are passed, it is clearer not to handle default arguments.

In BOUT++ there are always the same number of guard cells and boundary cells in a given direction, so do not need to specify the number of boundary cells separately.

Merge master into #30 to fix conflicts in making boundary cells optional

pep8speaks · 2019-07-25T20:26:52Z

Hello @TomNicholas! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-07-26 12:26:59 UTC

ZedThree · 2019-07-26T08:12:24Z

xbout/load.py

+    guards : dict
+        Number of guard cells along each dimension, e.g. {'x': 2, 't': 0}
+    keep_boundaries : dict
+        Whether or not to preserve the boundary cells along each dimension, e.g.


Just for reference, nxpe, nype aren't documented here

Also, I guess guards and keep_boundaries are optional, although that is somewhat obvious from the signature (if you're familiar with the syntax!)

I've added nxpe and nype to the docstring.

guards and keep_boundaries aren't optional. The foo(*, a, b) syntax means that a and b are required keyword arguments. I found it recently and quite like it because I think long sequences of positional arguments are bug-prone, but it's still nice to use required arguments sometimes - for example here it's an internal-implementation-only function which isn't called with default arguments, so no need to have the handling for default arguments (that would be needed if we had something like guards=None).

Merge master into #30 to fix conflicts in making boundary cells optional

Optionally keep guard cells

TomNicholas added 3 commits December 19, 2018 20:33

Wrote function to determine if dataset contains guard cells

2e4c15e

Refactored tests of the logic for checking presence of guards

2a79b61

Now successfully keeps x guard cells, more tests required though

602f557

TomNicholas added the enhancement New feature or request label Mar 20, 2019

TomNicholas added this to the 1.0 - First official release milestone Mar 20, 2019

TomNicholas requested a review from ZedThree March 20, 2019 17:12

ZedThree requested changes Jul 2, 2019

View reviewed changes

johnomotani added 6 commits July 22, 2019 21:35

Merge branch 'master' into keep_guard_cells

4a292ee

Fix test_trim_timing_info

017e7b3

_trim was missing a required argument.

Use '=None' rather than '={}' for default arguments in _trim

dfabc7a

Make all arguments to _trim non-optional

50a4afa

Since _trim is a private method, and in the only place it is used all the arguments are passed, it is clearer not to handle default arguments.

Remove separate guard and boundary cell count inputs

8301fe0

In BOUT++ there are always the same number of guard cells and boundary cells in a given direction, so do not need to specify the number of boundary cells separately.

Docstrings for keep_xboundaries and keep_yboundaries in open_boutdataset

9f857a6

This was referenced Jul 22, 2019

Merge master into #30 #39

Merged

Allow keeping y boundary cells at second divertor #40

Merged

johnomotani and others added 2 commits July 23, 2019 09:17

Fix PEP8 warnings

b4d8a23

Merge pull request #39 from boutproject/keep_guard_cells-merge-master

99f6d2b

Merge master into #30 to fix conflicts in making boundary cells optional

ZedThree reviewed Jul 26, 2019

View reviewed changes

ZedThree approved these changes Jul 26, 2019

View reviewed changes

Document nxpe and nype arguments to _trim

82ff092

johnomotani approved these changes Jul 26, 2019

View reviewed changes

TomNicholas merged commit 3886da9 into master Jul 29, 2019

johnomotani deleted the keep_guard_cells branch August 10, 2019 18:43

johnomotani pushed a commit that referenced this pull request Dec 11, 2019

Merge pull request #39 from boutproject/keep_guard_cells-merge-master

ccc86c1

Merge master into #30 to fix conflicts in making boundary cells optional

johnomotani pushed a commit that referenced this pull request Dec 11, 2019

Merge pull request #30 from boutproject/keep_guard_cells

32be3d7

Optionally keep guard cells

johnomotani mentioned this pull request Mar 26, 2020

Option to retain boundary/guard cells #19

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optionally keep guard cells #30

Optionally keep guard cells #30

TomNicholas commented Mar 20, 2019 •

edited

Loading

johnomotani commented Mar 20, 2019

TomNicholas commented Mar 20, 2019

codecov-io commented Mar 20, 2019 •

edited

Loading

ZedThree Mar 22, 2019

ZedThree Jul 2, 2019

TomNicholas Jul 2, 2019

ZedThree Jul 2, 2019

TomNicholas Jul 2, 2019

ZedThree Jul 2, 2019

TomNicholas Jul 25, 2019

ZedThree commented Jul 2, 2019

TomNicholas commented Jul 2, 2019

ZedThree commented Jul 2, 2019

TomNicholas commented Jul 2, 2019

bendudson commented Jul 2, 2019

pep8speaks commented Jul 25, 2019 •

edited

Loading

ZedThree Jul 26, 2019

johnomotani Jul 26, 2019

Optionally keep guard cells #30

Optionally keep guard cells #30

Conversation

TomNicholas commented Mar 20, 2019 • edited Loading

johnomotani commented Mar 20, 2019

TomNicholas commented Mar 20, 2019

codecov-io commented Mar 20, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZedThree commented Jul 2, 2019

TomNicholas commented Jul 2, 2019

ZedThree commented Jul 2, 2019

TomNicholas commented Jul 2, 2019

bendudson commented Jul 2, 2019

pep8speaks commented Jul 25, 2019 • edited Loading

Comment last updated at 2019-07-26 12:26:59 UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomNicholas commented Mar 20, 2019 •

edited

Loading

codecov-io commented Mar 20, 2019 •

edited

Loading

pep8speaks commented Jul 25, 2019 •

edited

Loading