Feature/issue-142: duplicated dimension error with TEMPO ozone profile #141

danielfromearth · 2023-02-06T16:13:30Z

Github Issue: #142

Description

Modified the remove_duplicate_dims() function so that the subsetter pipeline does not fail when encountering the duplicated "layer" dimension in TEMPO ozone profile data (variables: support_data/ozone_averaging_kernel and support_data/ozone_noise_correlation_matrix)

Overview of work done

Added checks in remove_duplicate_dims() for whether the Dimension and Variable corresponding to the duplicated dimension already exist in the NetCDF. The function directly writes the new variable with no duplicated dimensions rather than keeping it with an altered name that needs to be renamed later in the subsetting procedure.

Overview of verification done

Added a new unit test for TEMPO ozone profile data. Checked that all automated tests passed successfully.

Overview of integration done

Explain how this change was integration tested. Provide screenshots or logs if appropriate. An example of this would be a local Harmony deployment.

PR checklist:

Linted
Updated unit tests
Updated changelog
Integration testing

See Pull Request Review Checklist for pointers on reviewing this pull request

…nsion)

…ataset()`" This reverts commit e7b7096.

danielfromearth · 2023-02-07T16:08:48Z

@nlenssen2013, @frankinspace, I've made some edits to the remove_duplicate_dims() function in this branch in order to allow the subsetter to work with TEMPO Ozone proxy data files, which have duplicated dimensions in two of the variables. This change seems to be working well without the need to hold off variable renaming (with rename_dup_vars() using xarray) later in the subset pipeline. I'm a bit unsure whether this will work for all cases though. Are there additional files we can do regression tests with, besides the ones used by the automated tests in the tests/data directory?

nlenssen2013 · 2023-02-07T19:05:41Z

@nlenssen2013, @frankinspace, I've made some edits to the remove_duplicate_dims() function in this branch in order to allow the subsetter to work with TEMPO Ozone proxy data files, which have duplicated dimensions in two of the variables. This change seems to be working well without the need to hold off variable renaming (with rename_dup_vars() using xarray) later in the subset pipeline. I'm a bit unsure whether this will work for all cases though. Are there additional files we can do regression tests with, besides the ones used by the automated tests in the tests/data directory?

To my knowledge, GESDISC only has two collections with this duplicate dimension issue in SNDR and Tropomi. Both of those files get tested in unit test.

nlenssen2013 · 2023-02-07T19:44:29Z

podaac/subsetter/dimension_cleanup.py

    # return the variables that will need to be renamed: Rename method is still an issue per https://github.com/Unidata/netcdf-c/issues/1672
-    return nc_dataset, dup_new_varnames
+    return nc_dataset, []


If the new variable names with duplicate dimensions are the same as the original name (without the '_1' at the end) then a blank list doesn't need to be returned and the function below should be removed. Will need to check with our products.

Yep! I've updated the branch now to remove that return value and the use of the other renaming function.

…_as_nc_dataset()`"" This reverts commit 3fe9c2a.

frankinspace

Looks good to me. I think if the pytests all succeeded we are reasonably confident it hasn't broken any of the other datasets. Nice work!

danielfromearth · 2023-02-16T16:32:30Z

Hey all, just checking back on this...
@nlenssen2013, @jamesfwood, are you still planning to do additional checks with GESDISC or other data products before we move this from the fork into the base repository? (or perhaps you've already done additional checks?)

nlenssen2013 · 2023-02-16T16:40:35Z

Hey all, just checking back on this... @nlenssen2013, @jamesfwood, are you still planning to do additional checks with GESDISC or other data products before we move this from the fork into the base repository? (or perhaps you've already done additional checks?)

Yeah, it checks out with our datasets fine, the second function is still needed for one of our collections so the merged branch into develop is solid

danielfromearth · 2023-02-16T18:10:18Z

Hey all, just checking back on this... @nlenssen2013, @jamesfwood, are you still planning to do additional checks with GESDISC or other data products before we move this from the fork into the base repository? (or perhaps you've already done additional checks?)

Yeah, it checks out with our datasets fine, the second function is still needed for one of our collections so the merged branch into develop is solid

@nlenssen2013, you mean rename_dup_vars() is still needed? Just double-checking with you, since the latest version of this branch no longer uses that function at all (actually, I was thinking the function could now be deleted too) — is that okay?

If it's okay, I'll just go ahead and squash+merge this. If not, we might need to make a few more tweaks to use that rename_dup_vars() function again.

nlenssen2013 · 2023-02-16T19:58:58Z

Hey all, just checking back on this... @nlenssen2013, @jamesfwood, are you still planning to do additional checks with GESDISC or other data products before we move this from the fork into the base repository? (or perhaps you've already done additional checks?)

Yeah, it checks out with our datasets fine, the second function is still needed for one of our collections so the merged branch into develop is solid

@nlenssen2013, you mean rename_dup_vars() is still needed? Just double-checking with you, since the latest version of this branch no longer uses that function at all (actually, I was thinking the function could now be deleted too) — is that okay?

If it's okay, I'll just go ahead and squash+merge this. If not, we might need to make a few more tweaks to use that rename_dup_vars() function again.

I'm not getting all the pytest tests/ to pass without it currently, not sure if you are. Not a problem for me if it gets merged though - was able to run an individual subset on that collection and it looks good.

danielfromearth · 2023-02-16T22:00:31Z

@nlenssen2013. hmmm, I just reran using pytest, and all of the tests in tests/ passed on my laptop from this branch (danielfromearth:feature/currently-no-issue-duplicated-layer-dim). If they aren't passing for you, are you definitely using the same branch and pulled the latest?

Perhaps we merge this into the podaac:feature branch, and then let the automated test suite — run via GitHub actions — serve as the more authoritative check before merging it into develop.

nlenssen2013 · 2023-02-17T15:58:32Z

@nlenssen2013. hmmm, I just reran using pytest, and all of the tests in tests/ passed on my laptop from this branch (danielfromearth:feature/currently-no-issue-duplicated-layer-dim). If they aren't passing for you, are you definitely using the same branch and pulled the latest?

Perhaps we merge this into the podaac:feature branch, and then let the automated test suite — run via GitHub actions — serve as the more authoritative check before merging it into develop.

Yep, I'm fine with that

#148) * Feature/issue-142: duplicated dimension error with TEMPO ozone profile (#141) * add test for ozone profile proxy data * rework duplicate dimension removal to work with TEMPO ozone profile data * pylint update * simplify return of `remove_duplicate_dims() and `open_as_nc_dataset()` * remove unused import per pylint * add test data files for TEMPO NO2 and O3PROF (contains duplicate dimension) * clean up comments * Revert "simplify return of `remove_duplicate_dims() and `open_as_nc_dataset()`" This reverts commit e7b7096. * include Tuple import * update CHANGELOG.md * Revert "Revert "simplify return of `remove_duplicate_dims() and `open_as_nc_dataset()`"" This reverts commit 3fe9c2a. * remove unused import for flake8 * remove now-unused `rename_dup_vars()` function

danielfromearth added 2 commits February 6, 2023 10:00

add test for ozone profile proxy data

cd4317b

rework duplicate dimension removal to work with TEMPO ozone profile data

632fc51

danielfromearth changed the title ~~Feature/currently no issue duplicated layer dim~~ Feature/currently no issue - duplicated dimension for TEMPO ozone Feb 6, 2023

danielfromearth added 4 commits February 6, 2023 11:21

pylint update

3e095d9

simplify return of remove_duplicate_dims() and open_as_nc_dataset()`

e7b7096

remove unused import per pylint

492d505

add test data files for TEMPO NO2 and O3PROF (contains duplicate dime…

2d4a560

…nsion)

danielfromearth linked an issue Feb 6, 2023 that may be closed by this pull request

Duplicated dimension in TEMPO ozone profile proxy data raises error #142

Closed

danielfromearth changed the title ~~Feature/currently no issue - duplicated dimension for TEMPO ozone~~ Feature/issue-142 duplicated dimension for TEMPO ozone Feb 6, 2023

danielfromearth changed the title ~~Feature/issue-142 duplicated dimension for TEMPO ozone~~ Feature/issue-142: duplicated dimension error with TEMPO ozone profile Feb 6, 2023

danielfromearth added 3 commits February 6, 2023 12:48

clean up comments

8d3061d

Revert "simplify return of remove_duplicate_dims() and open_as_nc_d…

3fe9c2a

…ataset()`" This reverts commit e7b7096.

include Tuple import

6ce6be1

danielfromearth marked this pull request as ready for review February 7, 2023 16:00

danielfromearth requested review from nlenssen2013 and frankinspace February 7, 2023 16:03

danielfromearth changed the base branch from develop to feature/issue-142-duplicated-dimensions-in-tempo-ozone-profile February 7, 2023 16:29

update CHANGELOG.md

7a82883

nlenssen2013 reviewed Feb 7, 2023

View reviewed changes

nlenssen2013 requested a review from jamesfwood February 8, 2023 19:54

Revert "Revert "simplify return of remove_duplicate_dims() and open…

c944e02

…_as_nc_dataset()`"" This reverts commit 3fe9c2a.

frankinspace approved these changes Feb 13, 2023

View reviewed changes

nlenssen2013 approved these changes Feb 16, 2023

View reviewed changes

danielfromearth merged commit 2bf1a2d into podaac:feature/issue-142-duplicated-dimensions-in-tempo-ozone-profile Feb 17, 2023

danielfromearth deleted the feature/currently-no-issue-duplicated-layer-dim branch March 1, 2023 14:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/issue-142: duplicated dimension error with TEMPO ozone profile #141

Feature/issue-142: duplicated dimension error with TEMPO ozone profile #141

danielfromearth commented Feb 6, 2023 •

edited by frankinspace

Loading

danielfromearth commented Feb 7, 2023 •

edited

Loading

nlenssen2013 commented Feb 7, 2023

nlenssen2013 Feb 7, 2023

danielfromearth Feb 8, 2023

frankinspace left a comment

danielfromearth commented Feb 16, 2023

nlenssen2013 commented Feb 16, 2023

danielfromearth commented Feb 16, 2023 •

edited

Loading

nlenssen2013 commented Feb 16, 2023

danielfromearth commented Feb 16, 2023 •

edited

Loading

nlenssen2013 commented Feb 17, 2023

Feature/issue-142: duplicated dimension error with TEMPO ozone profile #141

Feature/issue-142: duplicated dimension error with TEMPO ozone profile #141

Conversation

danielfromearth commented Feb 6, 2023 • edited by frankinspace Loading

Description

Overview of work done

Overview of verification done

Overview of integration done

PR checklist:

danielfromearth commented Feb 7, 2023 • edited Loading

nlenssen2013 commented Feb 7, 2023

nlenssen2013 Feb 7, 2023

Choose a reason for hiding this comment

danielfromearth Feb 8, 2023

Choose a reason for hiding this comment

frankinspace left a comment

Choose a reason for hiding this comment

danielfromearth commented Feb 16, 2023

nlenssen2013 commented Feb 16, 2023

danielfromearth commented Feb 16, 2023 • edited Loading

nlenssen2013 commented Feb 16, 2023

danielfromearth commented Feb 16, 2023 • edited Loading

nlenssen2013 commented Feb 17, 2023

danielfromearth commented Feb 6, 2023 •

edited by frankinspace

Loading

danielfromearth commented Feb 7, 2023 •

edited

Loading

danielfromearth commented Feb 16, 2023 •

edited

Loading

danielfromearth commented Feb 16, 2023 •

edited

Loading