ENH: Adding cfradial frontend.

[zssherman]

PR Summary

This pull request contains the commits from @ngoldbaum , @matthewturk and I for the cfradial frontend. The new front ends allows for a radar file to be loaded using yt. Unit tests have been written for the frontend. I had to do a revert because I forgot I had the plot callback in this branch, so I reverted the commit, and have the plot callback code in a separate branch for a future pull request.

PR Checklist

• Code passes flake8 checker
• New features are documented, with docstrings and narrative docs
• Adds a test for any bugs fixed. Adds tests for new features.

[neutrinoceros]

Hi @zssherman. I'm willing to give you a hand in getting this merged.
The first issue I'm seeing with the current state of the code base is that you're modifying a file that is not part of yt anymore, and now belongs in unyt. @jzuhone should be able to guide you through migrating this to the other project.

Also it's been a while since the tests were run, and the infrastucture has changed quite a bit.
You might want to rebase your branch against the main branch (⚠️ , it used to be called master). Be aware you'll have to resolve conflicts, but they look pretty easy to me (except for the file that moved to a child project).

Note that we now use pre-commit to lint and auto-fix many nitpicks in code style, you might want to read this to learn more
https://yt-project.org/docs/dev/developing/developing.html#automatically-checking-and-fixing-code-style

Another quick remark is that we removed copyright headers a little while ago.

#-----------------------------------------------------------------------------
# Copyright (c) 2013, yt Development Team.
# ...

can you please remove the ones still in this PR ?

I will review this more in depth when you're ready. Don't hesitate to reach out here or on our Slack space if you need help !

[zssherman]

Hi @neutrinoceros , thanks for the information and taking a look at this! I will apply the changes you suggested and if I run into roadblocks let you know. I should be able to take a look at this again this weekend or early next week.

[chrishavlin]

This is great! It'll be great to get it in!

One quick comment: I'm not sure about the top level xarray import. We probably should use yt.utilities.on_demand_imports to avoid making xarray a hard dependency for yt. We actually ran into this with the nc4_cm1 frontend -- I initially added xarray to on_demand_imports (in this commit). Since we were just using xarray for reading fields it was actually simpler to use the existing netcdf handler yt.utilities.file_handler.NetCDF4FileHandler and I ended up removing that code before the frontend got merged. Since this frontend actually writes a processed .nc file it may warrant using xarray, but we should still avoid the top level import here.

In no way should this affect this PR, but there is actually a lot of overlap with the nc4_cm1 frontend. It make me think that a more general abstract netcdf frontend class would be helpful (I started one some months ago when starting on a frontend for seismic tomography... need to brush that off...).

[zssherman]

@neutrinoceros I believe I got most of the conflicts sorted out, and removed copywrite info. I also submitted a PR here for the unit fix. I wasn't entirely sure what to fix next.

[chrishavlin]

Nice work on sorting out the conflicts! Some initial comments, suggestions and questions below.

[chrishavlin]

Not sure about having an inline import here, might make more sense to also have pyart in on_demand_imports and import at the top of the file? @neutrinoceros will have thoughts...

[neutrinoceros]

I do ! see bellow :)

[zssherman]

Thank you @chrishavlin for taking a look! I've read your suggestions and will implement them soon. Yeah I believe when we originally wrote this I believe we were getting it to work on simpler or more general cases. I agree that changing the fields and grid parameter is needed as hardcoded the parameters is not ideal. I'll do those changes soon.

[zssherman]

@chrishavlin I believed I completed all your suggestions but the one I commented on. I added pyart in on demand imports as well, but with a suggestion from @neutrinoceros I can change where pyart is imported at. I believe the original intention was that it was only used in the section if the file wasn't a grid file. I believe I added both xarray to pyart to on_demand_imports correctly. Thanks again for the comments and suggestions! I can do more changes if needed.

[neutrinoceros]

can't review fully now but here's a suggestion ! keep up the good work

[chrishavlin]

Great! Thanks, @zssherman! one question: any idea if the test data, "CfRadialGrid/grid1.nc", has been uploaded to our testing server? We'll know once the tests start making it past the initial import but I suspect it's not been uploaded... Assuming it's not there, could you share a link to the file and we can get it where it needs to go? you can use yt at the command line to upload an archive of the CfRadialGrid directory with yt upload CfRadialGrid.tar.gz and it will upload the file to the yt hub and give you a link you can share here (docs on yt upload).

Edit: Zach, ignore this, the data is there already (see below).

[neutrinoceros]

@chrishavlin CfRadialGrid is registered on the server already :)
(see https://raw.githubusercontent.com/yt-project/website/master/data/datafiles.json)

edit: though loading it with yt.load_sample fails with

FileNotFoundError: No such file or directory: '...yt-project/testdata/CfRadialGrid/CfRadialGrid/grid1.nc'.

so there may be something missing indeed

edit 2: no wait, it's just a bug in the registry, I'll fix it

[chrishavlin]

oh interesting. the answer tests are missing their answers? I'm not sure why that's the case... but I think they should pass on re-running. But we can wait on any changes to the docs (or confirmation that they're good as is) before triggering.

[zssherman]

@chrishavlin No worries! So more details on cfradial here:
Cf/Radial is a CF compliant netCDF convention for radial data from radar and lidar platforms that supports both airborne and ground-based sensors. Because of its CF-compliance, CfRadial will allow researchers familiar with CF to read the data into a wide variety of analysis tools, models etc.

Essential the data is in polar coordinates. Utilizes range, azimuth and elevation in lat lon alt, and is then converted to x,y,z Cartesian. Py-ART's gridding uses interpolation to plot that data to a new grid. I can add this to the PR, if need be, would it be under that rst file you linked?

[chrishavlin]

Great! Thanks, @zssherman

Ya, if you could edit docs/source/examining/loading_data.rst so that it has more of that context/background that would be great! You should be able to just pull while on your cf-radial branch and get all the changes I've pushed in the past ~week. I'm also happy to take a stab at integrating your overview into what I wrote initially, but since you're the expert on the data here I'd rather defer to you. Just let me know!

For context, the current version of that page is rendered at https://yt-project.org/doc/examining/loading_data.html and it's where we mention all the various formats (the "frontends") that yt handles. For most of the yt frontends, it just gets straight into how to load data (I think it's assumed that folks trying to load their data know the background of their code/data source). But I'd prefer to follow the example of the Exodus II description on that page, where it starts with a sentence describing what the data format is.

[neutrinoceros]

the answer tests are missing their answers? I'm not sure why that's the case...

this is fixed by bumping the answers in tests/tests.yaml

I think it's assumed that folks trying to load their data know the background of their code/data source

correct. This page is more oriented toward specificities of loading data with respect to the common rules in yt. Most frontends require only a path to load, but there are cases where that's not sufficient.

[zssherman]

@chrishavlin I'll try to add in the doc stuff tomorrow, sadly got swamped.

[chrishavlin]

Great, thanks @zssherman ! I'll fix the final issues with the testing after you get those docs changes in.

[zssherman]

@chrishavlin I apologize for the lack of knowledge on this, but how would I update my local to this PR? Was going to update the docs but having conflict issues.

[zssherman]

oh wait nvm figured it out! Did the wrong command

[chrishavlin]

@zssherman I messaged you on the yt slack -- if you run into more issues feel free to reach out there. Totally ok to keep discussion here as well, but slack is sometimes easier for quick back and forth.

[matthewturk]

I have made a few suggestions, and asked a couple questions. However, I am totally fine with accepting this as-is, although I do think I want to think about the level values at some point. Nice work, everyone. Thank you.

[matthewturk]

I am genuinely pleased at how short this file is, given how complicated they often can be!

[chrishavlin]

looks like the updates to the grid_level values have changed the new answer tests. Not yet sure if the change is good or bad...

[matthewturk]

Can we verify them? It should be straightforward to do something like compute the volume, which might be related, or the path length.

[chrishavlin]

Ok, ran some comparisons and I can confirm that the current version is correct. Easiest to see this with projection plots. With the current version of the code, the following

import yt
ds = yt.load("CfRadialGrid/grid1.nc")
p = yt.ProjectionPlot(ds, "x", ("cf_radial", "reflectivity"), method="max")
p.set_log(("cf_radial", "reflectivity"), False)
p.set_cmap(("cf_radial", "reflectivity"), 'dusk')

results in this (very pretty) projection plot:

with the old version, the domain gets incorrectly scaled:

I'll bump the answer tests now!

One extra note: the projection plot above uses method="max" because the default (method='integrate') does not handle NaNs well and results in an all-NaN projection. See #3977

[neutrinoceros]

Alright, I think this is now complete. I will let you guys deal with your remaining threads if you want to add anything still, but AFAIC it's good to go.
Great job everyone !

[chrishavlin]

Just resolved the remaining threads -- @matthewturk wanna refresh your review?

[welcome]

Hooray! Congratulations on your first merged pull request! We hope we keep seeing you around! 🎆

[zssherman]

Thank you all for all your help with this!

[matthewturk]

Thank *you* for your patience and hard work!

…

On Fri, Jun 17, 2022 at 10:56 AM Zach Sherman ***@***.***> wrote: Thank you all for all your help with this! — Reply to this email directly, view it on GitHub <#1990 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAVXO466BRJHBXSR7G4R2LVPSNY7ANCNFSM4FR2DKJA> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>