Some changes for FCI #251

dschwoerer · 2022-09-30T10:57:23Z

f08d14d is not FCI related, but I hope to add soon attributes to zoidberg grids, in which case it would be great if that would be preserved ...

codecov-commenter · 2022-09-30T11:31:28Z

Codecov Report

Merging #251 (07ebb9f) into master (280a570) will decrease coverage by 1.93%.
The diff coverage is 3.15%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master     #251      +/-   ##
==========================================
- Coverage   69.01%   67.09%   -1.93%     
==========================================
  Files          15       16       +1     
  Lines        3208     3300      +92     
  Branches      790      802      +12     
==========================================
  Hits         2214     2214              
- Misses        732      822      +90     
- Partials      262      264       +2

Impacted Files	Coverage Δ
xbout/tracing.py	`0.00% <0.00%> (ø)`
xbout/geometries.py	`69.58% <75.00%> (-0.37%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

johnomotani

Thanks @dschwoerer, looks nice!

Is it worth adding a method to BoutDataset so that if a dataset has FCI geometry it's possible to get a plot from a one-line call, something like ds.bout.poincare_plot()?

It would be lovely to have a test for the field-line tracing, but I guess that could be pretty complicated to set up.

johnomotani · 2022-10-26T09:27:32Z

xbout/tracing.py

@@ -0,0 +1,115 @@
+import numpy as np
+import eudist


Should we add eudist as a dependency? Also would be nice if it could be added to conda-forge, for those of us who use conda 😃

Probably. I thought of maybe an extra [fci] section?
I have no experience with conda. Do you want to do it?

Probably. I thought of maybe an extra [fci] section?

Sounds good!

I have no experience with conda. Do you want to do it?

No prob. I'll set it up and make you a maintainer on the 'feedstock' repo. It's easy to maintain once it's set up, just click 'merge' on auto-generated PRs that're triggered by releases to PyPi and update the dependency list if it changes.

johnomotani · 2022-10-26T09:47:33Z

xbout/tracing.py

+from warnings import warn
+
+
+def rz_to_ab(rz, mesh):


Some comment about what this function does would be useful. Looks like a Newton iteration? Finds position within a cell in grid-index space? Using some sort of linear algebra on basis vectors(?) describing the cell?

johnomotani · 2022-10-26T09:49:04Z

xbout/tracing.py

+            return albe, ij
+
+
+def ab_to_rz(ab, ij, mesh):


Comment on what this does. Grid-index space -> spatial position? What are a, b, c?

johnomotani · 2022-10-26T09:51:19Z

xbout/tracing.py

+
+
+def setup_mesh(x, y):
+    def per(d):


Could maybe call this function apply_periodicity() to be slightly easier to read?

johnomotani · 2022-10-26T09:53:05Z

xbout/tracing.py

+    return mymesh(x, y)
+
+
+class mymesh(eudist.PolyMesh):


Maybe PascalCase form - MyMesh - as it's a class?

Also a brief docstring would be nice.

johnomotani · 2022-10-26T09:57:08Z

xbout/tracing.py

+    return A + al * a + be * b + al * be * c
+
+
+def setup_mesh(x, y):


Add docstring? E.g. something like "Creates a MyMesh object from R and Z coordinates..." and also explain why the periodicity thing is needed (I guess because the z-dimension of the grid is assumed to be periodic?).

johnomotani · 2022-10-26T09:58:05Z

xbout/tracing.py

+        self.shape = tuple([x - 1 for x in x.shape])
+
+
+class Tracer:


Docstring would be nice. How does this work? Somehow relies on the field line tracing that was already done by Zoidberg to make the grid file?

johnomotani · 2022-10-26T10:01:36Z

xbout/tracing.py

+            )
+        self.meshes = meshes
+
+    def poincare(self, rz, yind=0, num=100, early_exit="warn"):


Please add a docstring that explains the arguments.

johnomotani · 2022-10-26T10:18:48Z

xbout/tracing.py

+
+def rz_to_ab(rz, mesh):
+    ij = mesh.find_cell(rz)
+    assert ij >= 0, "We left the domain"


I think this should be an actual exception - maybe define an OutOfDomainError or something? If for some reason someone runs Python in 'optimised' mode with -O, all assert statements just get ignored (https://docs.python.org/3/reference/simple_stmts.html#the-assert-statement), so they should only be used for debugging, not program logic.

johnomotani · 2022-10-26T11:05:25Z

xbout/tracing.py

+            for d, meshes in enumerate(thismeshes):
+                try:
+                    abij = rz_to_ab(rz, meshes[0])
+                except AssertionError as e:


Note: will need updating if the exception is changed as suggested above.

dschwoerer · 2022-10-27T21:23:16Z

Is it worth adding a method to BoutDataset so that if a dataset has FCI geometry it's possible to get a plot from a one-line call, something like ds.bout.poincare_plot()?

Do you know how to do that? That certainly would be nice.

It would be lovely to have a test for the field-line tracing, but I guess that could be pretty complicated to set up.

Test would be easy if we had an example grid. Can we download one for the tests? At least for CI it should be no issue. We could host it on zenodo or at the mpcdf gitlab or something else?

johnomotani · 2022-10-28T09:35:30Z

For a method, in BoutDataset add something like (?)

"""
docstring...
"""
def poincare_plot(
    self,
    rz,
    yind=0,
    num=100,
    early_exit="warn",
    direction="forward",
    new_fig=True,
    show=False,
    save_as=None
):
    ds = self.data
    mytracer = Tracer(ds, direction=direction)
    rz = mytracer.poincare(rz=rz, yind=yind, num=num, early_exit=early_exit)

    if new_fig:
        plt.figure()
    p = plt.scatter(rz)
    if save_as:
        plt.savefig(save_as)
    if show:
        plt.show()

    return p

Probably need to also set some default arguments so that the plot looks nice. Maybe also have a scatter_kwargs=None argument that lets a dict be passed to tweak the plot?

johnomotani · 2022-10-28T09:48:25Z

About the grid file for testing: Indeed don't want to add a binary file to the install, so would be a test that only runs from the source repo. We could add another directory like extra_tests so it doesn't get installed. For managing the file, we could either add it to the repo with git-lfs, or put it on Zenodo and wget it. I think git-lfs is simpler, and I guess it won't be a huge file (?) - I don't think developers would mind downloading 100MB or so...

dschwoerer · 2022-10-28T09:55:53Z

Oh sorry, I misread. I thought you wanted to only add the method if it is an fci dataset.

Concerning git-lfs, I do not like the github one, they have not only strict size limits, which should be fine, but also limits on traffic, so the fci grid repo is already putting me at the limit ...
So I will go with zenodo + caching, if you do not like the mpcdf gitlab approach, which I use for xemc3 ...

johnomotani · 2022-10-28T10:17:59Z

Oh sorry, I misread. I thought you wanted to only add the method if it is an fci dataset.

That would be complicated! Probably should add some check that the dataset is an fci one in the method though.

If it's not a lot more complicated, I think I'd prefer Zenodo to mpcdf-gitlab, as it's not dependent on having someone with an mpcdf account to keep it going.

The forward/backward_{R,Z} are needed for field line tracing

* Rename internal stuff with _ * Use OutOfDomainError if we leave the domain * Add doc strings

Currently we only need eudist additionally

dschwoerer · 2023-02-02T11:55:48Z

Still missing:

tests
dataset accessor

dschwoerer marked this pull request as ready for review September 30, 2022 10:57

johnomotani reviewed Oct 26, 2022

View reviewed changes

dschwoerer added 5 commits February 2, 2023 11:21

Preserve attributes

f7c5b2c

Add more data from the grid

e67ac3a

The forward/backward_{R,Z} are needed for field line tracing

Add field line tracer for FCI

252b5cf

Cleanup tracing

61e8129

* Rename internal stuff with _ * Use OutOfDomainError if we leave the domain * Add doc strings

Add fci section to dependencies

07ebb9f

Currently we only need eudist additionally

dschwoerer force-pushed the fci branch from 1300d4d to 07ebb9f Compare February 2, 2023 10:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some changes for FCI #251

Some changes for FCI #251

dschwoerer commented Sep 30, 2022

codecov-commenter commented Sep 30, 2022 •

edited

Loading

johnomotani left a comment

johnomotani Oct 26, 2022

dschwoerer Oct 26, 2022

johnomotani Oct 27, 2022

johnomotani Oct 26, 2022

johnomotani Oct 26, 2022

johnomotani Oct 26, 2022

johnomotani Oct 26, 2022

johnomotani Oct 26, 2022

johnomotani Oct 26, 2022

johnomotani Oct 26, 2022

johnomotani Oct 26, 2022

johnomotani Oct 26, 2022

dschwoerer commented Oct 27, 2022

johnomotani commented Oct 28, 2022 •

edited

Loading

johnomotani commented Oct 28, 2022

dschwoerer commented Oct 28, 2022

johnomotani commented Oct 28, 2022

dschwoerer commented Feb 2, 2023

		return A + al * a + be * b + al * be * c


		def setup_mesh(x, y):

Some changes for FCI #251

Are you sure you want to change the base?

Some changes for FCI #251

Conversation

dschwoerer commented Sep 30, 2022

codecov-commenter commented Sep 30, 2022 • edited Loading

Codecov Report

johnomotani left a 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

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

dschwoerer commented Oct 27, 2022

johnomotani commented Oct 28, 2022 • edited Loading

johnomotani commented Oct 28, 2022

dschwoerer commented Oct 28, 2022

johnomotani commented Oct 28, 2022

dschwoerer commented Feb 2, 2023

codecov-commenter commented Sep 30, 2022 •

edited

Loading

johnomotani commented Oct 28, 2022 •

edited

Loading