search

podaac / l2ss-py

Level 2 subsetter with Harmony integration
https://podaac.github.io/l2ss-py/
Apache License 2.0
11 stars 11 forks source link

Feature/issue 155 #158

Closed nlenssen2013 closed 1 year ago

nlenssen2013 commented 1 year ago

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:

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

jamesfwood commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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?