-
Notifications
You must be signed in to change notification settings - Fork 107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add read-in functionality for deeply nested variables #281
Conversation
…length var and varpath lists
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
👈 Launch a binder notebook on this branch for commit 763305b I will automatically update this comment whenever this PR is modified 👈 Launch a binder notebook on this branch for commit 1e8b785 👈 Launch a binder notebook on this branch for commit c385251 👈 Launch a binder notebook on this branch for commit 39a04ce 👈 Launch a binder notebook on this branch for commit c3287b8 👈 Launch a binder notebook on this branch for commit 97a70b2 |
Codecov Report
@@ Coverage Diff @@
## development #281 +/- ##
===============================================
- Coverage 55.47% 55.06% -0.42%
===============================================
Files 31 31
Lines 2017 2034 +17
Branches 413 420 +7
===============================================
+ Hits 1119 1120 +1
- Misses 828 844 +16
Partials 70 70
Continue to review full report at Codecov.
|
I'm not familiar with ATL08, so I'd recommend @linxiongecu check to make sure things work (perhaps try P.S. If you're interested in a deep rabbit hole on nested variable support in |
I did visit this rabbit hole earlier this week. An interesting read. @linxiongecu, are you available to review this PR? I can help you through that process if that helps. |
Hi everyone, I was able to replicate the error that @linxiongecu mentioned here (#277 (comment)). For me when debugging with PyCharm, the kernel kill is like this:
Which seems to be a segfault (according to this StackOverflow topic). While debugging with breakpoints, the segfault happens when iterating on a for loop, Any ideas @JessicaS11 ? I wonder if this is inheriting a xarray structure and, therefore, such binding may be indeed hapenning. |
Thanks for digging into this @ricardobarroslourenco! You got me pointed in the right direction.
Close - the append there is adding an Xarray DataSet per file to a list of DataSets. After some painful line-by-line debugging, I found that the line causing the seg fault is line 369 in read.py. I grabbed the value of
I suspect it's related to the deprecation warning here. I wanted to share this update before signing off for the day. If anyone has code for a fix already written, feel free to add it to the PR, otherwise I will try to get something in tomorrow. |
Happy to help @JessicaS11 ! I just saw your recent commit on the fix, and going over it this afternoon. |
The fix should be in this branch. @linxiongecu @ricardobarroslourenco this PR is ready for review! |
Thanks @JessicaS11 ! I will get the updated debug, and see how it goes :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JessicaS11 I have reviewed all *.py
changes, and also the notebooks, and it seems that:
- They both address the ATL08 issues discussed in this PR (I have also downloaded locally, and it worked on my machine);
- The issues that @weiji14 mentioned for ATL06 (refer to manually remove 'z' from datetime string #294 (review) ) are already solved as he mentioned;
- The documentation and notebook changes seems also appropriate since there aren't main class/method changes, and they complement the examples provided.
Therefore, I will approve this PR.
@JessicaS11 , I have just approved this PR. Does this triggers automatically the rebuild of the library via Travis, and the versioning to |
@ricardobarroslourenco Thanks! Once this PR is merged into development, it will hang out there until the next release (which I'll be putting together this afternoon). After the development branch is merged into main and the release is tagged, that will trigger the builds and posting to PyPI and conda. |
Thanks, meanwhile, I am using the fix directly from this PR |
Hi, I got the index error on ATL08 similar to cases here:
If this is not the same bug, I can post a new issue. -) |
When reading in some deeper nested variables (here
canopy/h_canopy
) in ATL08, xarray does not read in the coordinates for the data which are at thecanopy
level. This caused merge issues when trying to build a DataSet of all wanted variables. This PR adds functionality that handles these more deeply nested variables by assigning them the proper coordinate values. It also fixes a few typos and an index error that was occurring during data read-in.