Skip to content
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

Feature/issue 155 #158

Merged
merged 34 commits into from
May 26, 2023
Merged

Feature/issue 155 #158

merged 34 commits into from
May 26, 2023

Conversation

nlenssen2013
Copy link
Contributor

Github Issue: #155

Description

Generalize the lat_var_prefix to compare against variable in the group.

Overview of work done

create get unique groups function. Loop through the lat var names and then determine what level the unique group that the latitude variable is located in.

Overview of verification done

Test a few different cases with latitudes at different levels in the file

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:

  • [x ] Linted
  • [x ] Updated unit tests
  • [ x] Updated changelog
  • Integration testing

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

@nlenssen2013 nlenssen2013 marked this pull request as ready for review May 19, 2023 21:36
@nlenssen2013 nlenssen2013 requested a review from jamesfwood May 23, 2023 18:19
@jamesfwood
Copy link
Collaborator

Hi @nlenssen2013. Does this mean that the latitude and longitude variables are inside of groups? And there could be multiple lat/lon variables? I'm having a little trouble following your description and example. Thanks!

@nlenssen2013
Copy link
Contributor Author

Hi @nlenssen2013. Does this mean that the latitude and longitude variables are inside of groups? And there could be multiple lat/lon variables? I'm having a little trouble following your description and example. Thanks!

Yes, so get_coordinates gets the lat lon and time variables inside the groups for bounding box and temporal subsetting. Each variable will need to be assigned to one of those three for subsetting. This is where 'lat_var_prefix' comes into play. We can't over add variables, because then there will be duplicates, and we can't under assign variables and drop data. The variables are flattened with double underscores instead of '/' slashes. The current l2ss-py version assumes that the group above the latitude variable was main group for bounding box subsetting - hence [:-1] in the lat_var_prefix. MLS provided an issue that had lat and lon variables 2 levels up, and then sentinel 6 has lat and lon groups that are staggered in 'depth' in the file. The -1 for lat_var_prefix was always a hard code and needed to be generalized to find group level that makes that latitude group unique to the other latitude variables - might be 1,2,3 levels up for the same file. I provide some tests that have a set of lat variables and the expected lat prefix. Now that I look at it - I need to update the code slightly. __latitude, should return ''

@jamesfwood
Copy link
Collaborator

Sounds good Nick. Btw, it seems SonarCloud is failing. Did you add test cases to your new code to include it in the code coverage?

@nlenssen2013
Copy link
Contributor Author

Sounds good Nick. Btw, it seems SonarCloud is failing. Did you add test cases to your new code to include it in the code coverage?

Sounds good Nick. Btw, it seems SonarCloud is failing. Did you add test cases to your new code to include it in the code coverage?

Not sure why it's doing that. I call the new function, get_base_group_names, in the last test in the test file. Any thoughts? I'm trying to not add an entirely new granule if it's not needed for testing this one function

@jamesfwood
Copy link
Collaborator

Sounds good Nick. Btw, it seems SonarCloud is failing. Did you add test cases to your new code to include it in the code coverage?

Sounds good Nick. Btw, it seems SonarCloud is failing. Did you add test cases to your new code to include it in the code coverage?

Not sure why it's doing that. I call the new function, get_base_group_names, in the last test in the test file. Any thoughts? I'm trying to not add an entirely new granule if it's not needed for testing this one function

Sounds good Nick. Btw, it seems SonarCloud is failing. Did you add test cases to your new code to include it in the code coverage?

Sounds good Nick. Btw, it seems SonarCloud is failing. Did you add test cases to your new code to include it in the code coverage?

Not sure why it's doing that. I call the new function, get_base_group_names, in the last test in the test file. Any thoughts? I'm trying to not add an entirely new granule if it's not needed for testing this one function

Sounds good Nick. Btw, it seems SonarCloud is failing. Did you add test cases to your new code to include it in the code coverage?

Sounds good Nick. Btw, it seems SonarCloud is failing. Did you add test cases to your new code to include it in the code coverage?

Not sure why it's doing that. I call the new function, get_base_group_names, in the last test in the test file. Any thoughts? I'm trying to not add an entirely new granule if it's not needed for testing this one function

It might be just a weird error.

So is your PR ready to merge to develop?

Copy link
Collaborator

@danielfromearth danielfromearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

@nlenssen2013 nlenssen2013 merged commit 1ffe0cc into develop May 26, 2023
@nlenssen2013 nlenssen2013 deleted the feature/issue-155 branch May 26, 2023 20:33
jamesfwood added a commit that referenced this pull request Jul 19, 2023
* /version 2.5.0-alpha.0

* /version 2.5.0-alpha.1

* Adding collections OPS: C2599212091-POCLOUD

* /version 2.5.0-alpha.2

* Adding collections OPS: C2208421887-POCLOUD

* /version 2.5.0-alpha.3

* Adding collections OPS: C2205121449-POCLOUD

* /version 2.5.0-alpha.4

* update ascending flag variable clean up (#154)

* update ascending flag variable clean up

* Update changelog

* Add sndr ascending/descending type

* Update testing file

* added sndr file to list files to test

* change tests to call subset method

* change assertion test if box returned is true

* change assertion test if box returned is true

* expand time test zone

* Ascending flag does not coorespond to the asc_node_time

* Ascending flag does not coorespond to the asc_node_time

* Updated werkzeug=2.2.3 and cryptography=='39.0.1'

* Update poetry

* Ran poetry update

* Update poetry version used in pipeline

* Fix pylint errors

* Bump python version used in build

* rollback numpy=1.24 to numpy=1.23.5

---------

Co-authored-by: nlensse1 <nicholas.f.lenssen@nasa.gov>
Co-authored-by: Frank Greguska <89428916+frankinspace@users.noreply.github.com>

* /version 2.5.0a5

* Adding collections OPS: C2274919541-POCLOUD

* Adding collections UAT: C1238658392-POCLOUD C1243175554-POCLOUD

* /version 2.5.0a6

* Adding collections UAT: C1240817851-POCLOUD C1238621185-POCLOUD C1241042620-POCLOUD C1240739719-POCLOUD C1256420924-POCLOUD C1245295751-POCLOUD C1238621118-POCLOUD C1238621219-POCLOUD C1240739606-POCLOUD C1242735870-POCLOUD C1240739611-POCLOUD C1240739704-POCLOUD C1238657961-POCLOUD C1238658049-POCLOUD C1241042621-POCLOUD C1238621092-POCLOUD C1238658050-POCLOUD C1240739764-POCLOUD C1238621115-POCLOUD C1238657960-POCLOUD C1256524295-POCLOUD C1245295750-POCLOUD C1240739768-POCLOUD C1238621176-POCLOUD

* /version 2.5.0a7

* Adding collections UAT: C1238621178-POCLOUD C1238658088-POCLOUD C1238687534-POCLOUD C1238687284-POCLOUD C1256445396-POCLOUD C1238621172-POCLOUD C1240739654-POCLOUD C1238658086-POCLOUD C1238621186-POCLOUD C1238687546-POCLOUD C1238658080-POCLOUD C1240739688-POCLOUD C1238621182-POCLOUD

* /version 2.5.0a8

* Feature/podaac 5538 (#156)

* update l2ss to load each dataset variable individually to write to file

* remove unused variable

* remove test variable that is not used

* /version 2.5.0a9

* Adding collections OPS: C2296989383-POCLOUD C2296989388-POCLOUD C2296989390-POCLOUD

* /version 2.5.0a10

* Adding collections UAT: C1234208436-POCLOUD C1238570311-POCLOUD

* /version 2.5.0a11

* Adding collections UAT: C1256783391-POCLOUD C1256783386-POCLOUD

* /version 2.5.0a12

* Feature/podaac 5537 (#157)

* update l2ss py to open hdf5 files if we fail to open as netcdf, and change way we copy attributes in case it has a /

* fix pylint and removed comment

* update changelog

* /version 2.5.0a13

* Feature/podaac 5537 (#161)

* update l2ss py to open hdf5 files if we fail to open as netcdf, and change way we copy attributes in case it has a /

* fix pylint and removed comment

* update changelog

* change so that were able to process S1 and S2 dtypes

* fix pylint and flake8

* revert spaces

* remove spaces

* remove spaces

* /version 2.5.0a14

* Bump tornado from 6.2 to 6.3.2 (#160)

Bumps [tornado](https://github.com/tornadoweb/tornado) from 6.2 to 6.3.2.
- [Changelog](https://github.com/tornadoweb/tornado/blob/master/docs/releases.rst)
- [Commits](tornadoweb/tornado@v6.2.0...v6.3.2)

---
updated-dependencies:
- dependency-name: tornado
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump requests from 2.28.2 to 2.31.0 (#159)

Bumps [requests](https://github.com/psf/requests) from 2.28.2 to 2.31.0.
- [Release notes](https://github.com/psf/requests/releases)
- [Changelog](https://github.com/psf/requests/blob/main/HISTORY.md)
- [Commits](psf/requests@v2.28.2...v2.31.0)

---
updated-dependencies:
- dependency-name: requests
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* /version 2.5.0a15

* Feature/issue 155 (#158)

* add group variable fix test

* Updated MLS fix with TEMPO

* update linting

* fix pylinting

* Fix pylint

* fix pylint

* add extra blank line

* Update group_vars extending function

* fix linting

* attempted fix with variable subsetting

* pylinting updated

* Update subset.py and lint

* Update subset.py and lint

* fix unique groups

* fix unique groups

* fix unique groups

* fix unique groups

* add unit test

* remove print statements

* remove whitespace

* CHANGELOG updated

* add space in file

* remove whitespace

* Don't add a variable twice

* fix linting

* include test for latitude in root group

* include test for latitude in root group

* fix group handling

* fix group handling

* streamline group list control flow

* add missing import

* remove indent, remove print statement

---------

Co-authored-by: nlensse1 <nicholas.f.lenssen@nasa.gov>
Co-authored-by: jamesfwood <jamesfwood@hotmail.com>
Co-authored-by: danielfromearth <daniel.kaufman@nasa.gov>

* /version 2.5.0a16

* Updated changelog for release 2.5.0

* /version 2.6.0-alpha.0

* /version 2.5.0-rc.1

* trigger rebuild

* /version 2.5.0rc2

* /version 2.5.0rc3

* fix version format

* /version 2.5.0rc4

* Bugfix/fix docker versioning type (#164)

* update docker versioning to pep440

* update changelog

* /version 2.5.0rc5

* undo replacing slashes with _ in values for attributes (#165)

* /version 2.5.0rc6

* Bump cryptography from 39.0.2 to 41.0.0 (#167)

Bumps [cryptography](https://github.com/pyca/cryptography) from 39.0.2 to 41.0.0.
- [Changelog](https://github.com/pyca/cryptography/blob/main/CHANGELOG.rst)
- [Commits](pyca/cryptography@39.0.2...41.0.0)

---
updated-dependencies:
- dependency-name: cryptography
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* /version 2.6.0a1

* Feature/issue 162 (#166)

* add group variable fix test

* Updated MLS fix with TEMPO

* update linting

* fix pylinting

* Fix pylint

* fix pylint

* add extra blank line

* Update group_vars extending function

* fix linting

* attempted fix with variable subsetting

* pylinting updated

* Update subset.py and lint

* Update subset.py and lint

* fix unique groups

* fix unique groups

* fix unique groups

* fix unique groups

* add unit test

* remove print statements

* remove whitespace

* CHANGELOG updated

* add space in file

* remove whitespace

* Don't add a variable twice

* fix linting

* add root variable subsetting

* add root variable subsetting

* include file ext in tests for argument

* update tests

* update tests

* allow subsetting from one lat var name

* allow subsetting from one lat var name

* allow subsetting from one lat var name

* allow temporal subsetting for MLS and OCO3

* updated changelog

* Update test change

* remove itertools extra import

* Update checking the var_name

---------

Co-authored-by: nlensse1 <nicholas.f.lenssen@nasa.gov>

* /version 2.6.0a2

* Feature/issue 168 (#169)

* make separate copy of test data file to get expected results before subsetting

* update CHANGELOG.md

* lint the test_subset.py file via pylint

* /version 2.6.0a3

* /version 2.5.0rc7

* deploy

* /version 2.5.0rc8

* re-add get timevar process for MLS (#171)

* re-add get timevar process for MLS

* Update changelog

* Add comments for get time var name

* Add comments for get time var name

---------

Co-authored-by: nlensse1 <nicholas.f.lenssen@nasa.gov>

* /version 2.6.0a4

* /version 2.5.0rc9

* Adding collections UAT: C1256507988-POCLOUD

* Adding collections UAT: C1256507990-POCLOUD C1256122852-POCLOUD C1256507989-POCLOUD

* /version 2.6.0a5

* Feature/issue 173 (#174)

* Update xarray dataset merge

* update subsetting for variables without sptaital dimensions

* Update variable names and Changelog

* Update variable name

* Update variable name

---------

Co-authored-by: nlensse1 <nicholas.f.lenssen@nasa.gov>

* /version 2.6.0a6

* /version 2.5.0rc10

* Adding collections UAT: C1238621112-POCLOUD

* /version 2.6.0a7

* Adding collections OPS: C2601585875-POCLOUD C2628593693-POCLOUD C2601584109-POCLOUD C2601583089-POCLOUD C2601581863-POCLOUD C2628598397-POCLOUD

* /version 2.6.0a8

* /version 2.5.0rc11

* trigger redeploy

* /version 2.5.0rc13

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: l2ss-py bot <l2ss-py@noreply.github.com>
Co-authored-by: James Wood <James.F.Wood@jpl.nasa.gov>
Co-authored-by: podaac-cloud-dsa <podaac-cloud-dsa@jpl.nasa.gov>
Co-authored-by: Nick Lenssen <nicklenssen4@gmail.com>
Co-authored-by: nlensse1 <nicholas.f.lenssen@nasa.gov>
Co-authored-by: Frank Greguska <89428916+frankinspace@users.noreply.github.com>
Co-authored-by: sliu008 <69875423+sliu008@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: danielfromearth <daniel.kaufman@nasa.gov>
Co-authored-by: Daniel Kaufman <114174502+danielfromearth@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants