icesat2py / icepyx

Python tools for obtaining and working with ICESat-2 data
https://icepyx.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
203 stars 101 forks source link

fix ATL08 delta_time dimension read error #470

Closed JessicaS11 closed 8 months ago

JessicaS11 commented 8 months ago

Per #376, #466, and #469 ATL08 data could not be read in.

During a read in of one of the deeply nested group variables, delta_time was being turned into a dimension (rather than staying as coordinates), causing merge conflicts when added to portions of the dataset that only had delta_time as coordinates due to the conflicting sizes of the delta_time coordinate. By dropping the delta_time dimension, we are able to merge properly.

This PR is being submitted as a patch bug fix directly to main in order to create a patch 0.8.1 release for an upcoming workshop.

github-actions[bot] commented 8 months ago

Binder :point_left: Launch a binder notebook on this branch for commit 9621b7f359f052e73b4ab7b8be54de9c9fa4ea63

I will automatically update this comment whenever this PR is modified

Binder :point_left: Launch a binder notebook on this branch for commit ee3242c3851a579eeb33801ba33b9c9b827b00ca

rwegener2 commented 8 months ago

Thanks for working on a fix @JessicaS11!

🎉

I tried this out and it runs smoothly for a single ATL08 file, multiple ATL08 files, and multiple ATL06 files. I'm getting an error trying to read ATL03 files, but I think that one may be a me problem because I get the same error using version 0.8.0.

😕

Looking at the output the ATL08 produces I am not seeing what I expected, but I also may just be misunderstanding. The output ds from the code in #469 (used reader.vars.append(beam_list=['gt1l', 'gt3r'], var_list=['h_canopy', "latitude", "longitude"])) was:

Screenshot 2023-11-14 at 9 06 06 AM

My confusions:

  1. Shouldn't h_canopy be a Data Variable?
  2. Shouldn't only gt1l and gt3r be listed in gt?

If you want to brainstorm together @JessicaS11 just let me know.

rwegener2 commented 8 months ago

As for the erroring build it seems like the error was a timeout on the visualization tests. Maybe it's an API issue? It's surprising that the API itself doesn't provide a timeout response. icepyx could write their own timeouts into it's code, so we get a recognizable error when the API called in the visualization code is non-responsive. But that, I think, would be a task for another day.

JessicaS11 commented 8 months ago

As for the erroring build it seems like the error was a timeout on the visualization tests. Maybe it's an API issue? It's surprising that the API itself doesn't provide a timeout response. icepyx could write their own timeouts into it's code, so we get a recognizable error when the API called in the visualization code is non-responsive. But that, I think, would be a task for another day.

I'll take the easy one first: this was addressed in #459. I like the idea of improving the timeouts/messaging on the icepyx side though.

JessicaS11 commented 8 months ago

My confusions:

1. Shouldn't `h_canopy` be a Data Variable?

2. Shouldn't only `gt1l` and `gt3r` be listed in `gt`?

Confusing indeed... I will see if I can make heads or tail of this.

UPDATE (#1): it's the is2ds.drop_dims("delta_time") line I added to deal with the merging issue; any variables added during that step are using delta_time, rather than photon_idx as their dimension, so the variables are dropped during the drop_dims. EDIT: This should be fixed!

UPDATE(#2): see #471. It appears to be an issue with all the beams being added when the mandatory variables are added. So if we dove into ds there would only be data for rgt and other default variables in the non-requested beams, but none for gt2r+h_canopy.