adding boundary condition #325

rcaneill · 2021-05-11T11:01:44Z

Aims to close #273

I added a section about the boundary conditions. I also corrected some minor typos.

rabernat

This is really fantastic @rcaneill! Thanks so much for your work.

Just one suggestion and one question. Otherwise LGTM!

doc/grids.rst

rabernat · 2021-05-11T11:43:13Z

doc/grids.rst

+    g
+
+We show here the value of the extra added point for 5 cases (extended, extrapolated, filled with 0, filled with 5,
+and periodic). The periodic condition is not an argument of the methods, but is provided


An API question: do you think we should change things so that all boundary conditions (including periodic) are handled in the same way?

Good question. I also asked myself about this while writing. Maybe just being able to overwrite the boundary condition at any time while calling the grid methods would be nice. So keep the periodic argument in the grid init, along with the boundary argument, but overwrite them when explicitly asked by user.

Fully in favour. See #195

I think we should actually do that, but have to go through a deprecation cycle.

jbusecke · 2021-05-11T13:42:18Z

This is really cool @rcaneill! Thank you very much for working on this.
There are a few issues that might be relevant here:

Since we demonstrate the extrapolate option, we should really clean up where and when we can apply that. This has been a 'semi-hidden' feature for a long time I think (fix boundary="extrapolate" #206)
boundary="periodic"? #195 seems relevant for the discussion started by @rabernat above. My two cents: I think this makes sense without loosing generality?

I had some other small nits for the code, but overall this looks amazing!

jbusecke · 2021-05-11T13:45:40Z

doc/grids.rst

+.. ipython:: python
+
+    def plot_bc(ds):
+        plt.plot(ds.x_g, g, marker='o', color='C6', label='g')


This code block (and the following), do not show up nicely in the Readthedocs preview (https://xgcm--325.org.readthedocs.build/en/325/grids.html) because the lines are too long.

See this example:

Could you shorten the lines? Ideally you would use black to format each code cell and then paste it back? I also wonder if there is a way to do this automatically. Ill open an issue for that in the future.

I just tried to use black (and copy/paste), but it does not seem to solve the problem

rabernat · 2021-05-11T14:07:07Z

The remaining issues with formatting are problems with our sphinx theme, not this PR.

rcaneill · 2021-05-11T14:10:59Z

So, should I just commit the 2 lines that are changed by black and that's it?

rabernat · 2021-05-11T14:24:53Z

Or just do nothing and we can deal with it in #326. 😄

jbusecke · 2021-05-11T14:46:56Z

Ahh 💩 seems like my black advice had some unintended consequences, and you reformatted some of the source code here.

Could you follow these steps, which should fix the source code again, but should not touch the docs.

Sorry for making this more complicated than necessary.

…to rcaneill_doc_boundary

jbusecke · 2021-07-30T22:30:47Z

I tried to fix the formatting here (since its good to go otherwise), but I am getting:

error: cannot format /Users/juliusbusecke/code/xgcm/doc/grids.rst: Cannot parse: 403:0: plt.grid(True)

it seems this part is upsetting blackdoc:

    @suppress
    plt.grid(True)
    @savefig grid_bc_extra_point.png
    plot_bc(ds)

@rcaneill do we necessarily need this? @andersy005 do you have an idea how to fix this?

rcaneill · 2021-07-31T08:06:29Z

Plotting the grid is not essential, it is just that I found it more clear and easier to refer to the value of each condition (as I choose the number to be integer and be located in the grid lines)

doc/grids.rst

Co-authored-by: keewis <keewis@users.noreply.github.com>

keewis · 2021-09-03T16:02:13Z

there is also a bug in blackdoc which is fixed in v0.3.4, so you'd need to update the version in .pre-commit-config.yaml to get it to pass

rcaneill · 2021-09-03T16:04:01Z

I don't know why it does not pass the code-style / pre-commit check now... The error is:

Traceback (most recent call last): File "/home/romain/.cache/pre-commit/repobuk3dc8i/py_env-python3/bin/blackdoc", line 5, in <module> from blackdoc.__main__ import main File "/home/romain/.cache/pre-commit/repobuk3dc8i/py_env-python3/lib/python3.9/site-packages/blackdoc/__main__.py", line 12, in <module> from .blackcompat import ( File "/home/romain/.cache/pre-commit/repobuk3dc8i/py_env-python3/lib/python3.9/site-packages/blackdoc/blackcompat.py", line 9, in <module> import toml ModuleNotFoundError: No module named 'toml'

keewis · 2021-09-03T16:05:52Z

yes, that's the bug I was talking about in #325 (comment). The fix is to upgrade the version of blackdoc.

rcaneill · 2021-09-04T16:50:37Z

I changed the code blocks so it passes all the tests and I am happy with the way it looks. I think it's ready for merging :)

jbusecke

Thank you for this excellent addition and the tremendous patience @rcaneill.

jbusecke · 2021-10-25T14:55:30Z

doc/grids.rst

+    g
+
+We show here the value of the extra added point for 5 cases (extended, extrapolated, filled with 0, filled with 5,
+and periodic). The periodic condition is not an argument of the methods, but is provided


I think we should actually do that, but have to go through a deprecation cycle.

jbusecke · 2021-10-25T14:57:13Z

Oh I forgot one thing! You should give yourself credit in docs/whats-new.rst @rcaneill. Then ill merge this immediately.

jbusecke

So sorry for the long wait again @rcaneill. Really appreciate your tenacity here. Good news is this will now be included in v0.6.0 (which we are releasing today).

Thanks again for this important contribution

rcaneill and others added 2 commits May 11, 2021 12:58

adding boundary condition

022608c

Minor style change

4092f0b

rabernat reviewed May 11, 2021

View reviewed changes

jbusecke reviewed May 11, 2021

View reviewed changes

jbusecke mentioned this pull request May 11, 2021

Apply black formatting to the docs #326

Closed

rcaneill added 3 commits May 11, 2021 16:54

minor black formating

06489c4

Merge branch 'rcaneill_doc_boundary' of github.com:rcaneill/xgcm-1 in…

e7cdf4f

…to rcaneill_doc_boundary

using numpy functions

f57789c

jbusecke self-assigned this Jul 6, 2021

Merge branch 'master' into rcaneill_doc_boundary

c21ccee

jdldeauna mentioned this pull request Jul 28, 2021

update whats new rst #363

Merged

1 task

keewis reviewed Aug 8, 2021

View reviewed changes

doc/grids.rst Outdated Show resolved Hide resolved

rcaneill and others added 3 commits September 3, 2021 17:19

Update doc/grids.rst

d49f709

Co-authored-by: keewis <keewis@users.noreply.github.com>

blackdoc complient

ed92dc0

needed to run black on test_transform.py

6a8e605

rcaneill added 5 commits September 4, 2021 16:08

fixing blackdoc bug

5ef82ad

removed trailing whitespace

bef8e68

fixing plotting issues

146fcb4

typo

ebb18f4

closing figure, needed so they don t sum up to each other

d430f05

passing pre commit

e04f4e1

rcaneill requested a review from jbusecke October 18, 2021 15:11

jbusecke approved these changes Oct 25, 2021

View reviewed changes

rcaneill added 2 commits October 25, 2021 17:28

credit for PR

efbe946

Merge branch 'master' into rcaneill_doc_boundary

1cf99f1

rcaneill requested a review from jbusecke November 1, 2021 16:02

jbusecke approved these changes Dec 3, 2021

View reviewed changes

jbusecke merged commit 742be12 into xgcm:master Dec 3, 2021

rcaneill mentioned this pull request Apr 21, 2022

[BUG (?)] 'boundary' condition unconsistent between e.g. grid.diff and grid.cumsum #485

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding boundary condition #325

adding boundary condition #325

rcaneill commented May 11, 2021

rabernat left a comment

rabernat May 11, 2021

rcaneill May 11, 2021

dcherian May 11, 2021

jbusecke Oct 25, 2021

jbusecke commented May 11, 2021

jbusecke May 11, 2021

rcaneill May 11, 2021

rabernat commented May 11, 2021

rcaneill commented May 11, 2021

rabernat commented May 11, 2021

jbusecke commented May 11, 2021

jbusecke commented Jul 30, 2021

rcaneill commented Jul 31, 2021

keewis commented Sep 3, 2021

rcaneill commented Sep 3, 2021

keewis commented Sep 3, 2021

rcaneill commented Sep 4, 2021

jbusecke left a comment

jbusecke Oct 25, 2021

jbusecke commented Oct 25, 2021

jbusecke left a comment

adding boundary condition #325

adding boundary condition #325

Conversation

rcaneill commented May 11, 2021

rabernat left a comment

Choose a reason for hiding this comment

rabernat May 11, 2021

Choose a reason for hiding this comment

rcaneill May 11, 2021

Choose a reason for hiding this comment

dcherian May 11, 2021

Choose a reason for hiding this comment

jbusecke Oct 25, 2021

Choose a reason for hiding this comment

jbusecke commented May 11, 2021

jbusecke May 11, 2021

Choose a reason for hiding this comment

rcaneill May 11, 2021

Choose a reason for hiding this comment

rabernat commented May 11, 2021

rcaneill commented May 11, 2021

rabernat commented May 11, 2021

jbusecke commented May 11, 2021

jbusecke commented Jul 30, 2021

rcaneill commented Jul 31, 2021

keewis commented Sep 3, 2021

rcaneill commented Sep 3, 2021

keewis commented Sep 3, 2021

rcaneill commented Sep 4, 2021

jbusecke left a comment

Choose a reason for hiding this comment

jbusecke Oct 25, 2021

Choose a reason for hiding this comment

jbusecke commented Oct 25, 2021

jbusecke left a comment

Choose a reason for hiding this comment