boutproject / xBOUT

Collects BOUT++ data from parallelized simulations into xarray.
https://xbout.readthedocs.io/en/latest/
Apache License 2.0
21 stars 10 forks source link

Some changes for FCI #251

Open dschwoerer opened 2 years ago

dschwoerer commented 2 years ago

f08d14d6ada93e1a32e084dd7a3501d07bfb0416 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 commented 2 years ago

Codecov Report

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

:mega: 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%) :arrow_down:

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

dschwoerer commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

dschwoerer commented 1 year ago

Still missing: