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

DAS-NONE: Move trajectory subsetter tests to shared_utils #104

Merged
merged 8 commits into from
Oct 15, 2024

Conversation

flamingbear
Copy link
Member

@flamingbear flamingbear commented Oct 12, 2024

Description

I saw a terrible example in the shared_utils readme linking a xarray githash. I cleaned that up, and thought let's just add trajectory subsetter to use the shared_utils since that's where the idea came from and that word was never going to get scheduled.

This PR updates Trajectory Subsetter test to use shared utilies, but also refactors them to remove the old datatree comparison and renames the old compare_results_to_reference_file_new -> compare_results_to_reference_file

This PR also separates out the basic shared utilities into utilities.py and compare.py so that if you're not using xarray comparisons you don't need to include it in your environment for your tests.

Jira Issue ID

None

Local Test Steps

Check out this branch. Build the trajecotry-subseter and nsidc-icesat2 images.

make trajectory-subsetter-image
make nsidc-icesat2-image

Set the appropriate environment variables for UAT and run the notebooks

# UAT
export HARMONY_HOST_URL=https://harmony.uat.earthdata.nasa.gov
export EDL_USER=
export EDL_PASSWORD=

./run_notebooks.sh trajectory-subsetter nsidc-icesat2

Verify they both succeed.

PR Acceptance Checklist

  • Acceptance criteria met
  • Tests added/updated (if needed) and passing
  • Documentation updated (if needed)
  • CHANGELOG updated with the changes for this PR

@flamingbear flamingbear changed the title DAS-NONE: Update README turns into refactor. DAS-NONE: Move trajectory subsetter tests to shared_utility (with shared_utility refactor) Oct 12, 2024
@flamingbear flamingbear changed the title DAS-NONE: Move trajectory subsetter tests to shared_utility (with shared_utility refactor) DAS-NONE: Move trajectory subsetter tests to shared_utility Oct 12, 2024
@flamingbear flamingbear marked this pull request as ready for review October 12, 2024 00:17
@flamingbear flamingbear changed the title DAS-NONE: Move trajectory subsetter tests to shared_utility DAS-NONE: Move trajectory subsetter tests to shared_utils Oct 12, 2024
Did not run it on all tests because I didn't want to release them all again.
It requires updating all of the tests and I don't have time for that.
Copy link
Contributor

@lyonthefrog lyonthefrog left a comment

Choose a reason for hiding this comment

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

I ran the tests and both notebooks passed.
image
I made a few simple simple comments and questions, so when those are resolved I'll approve!

test/shared_utils/README.md Outdated Show resolved Hide resolved


def remove_results_files() -> None:
"""Remove all NetCDF-4 files downloaded during the Swath Projector
Copy link
Contributor

Choose a reason for hiding this comment

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

HDF5 files in Trajectory Subsetter regression test? Also, could this be a shared utility or do we not really use it anywhere else yet?

Copy link
Member

@owenlittlejohns owenlittlejohns Oct 15, 2024

Choose a reason for hiding this comment

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

There is something similar in the #95 and, from the documentation string here, I'm going to go out on a limb and suggest maybe there is a similar function in the Swath Projector test utilities.

Copy link
Member Author

Choose a reason for hiding this comment

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

I commented below

I reckon the remove_results_files could be moved into the shared utilities (maybe with an optional argument or two, such as a directory to clean up, or a file extension), but I'm not going to hold the PR up over that.

I prefer using a temporaryDirectory, so I didn't move it into the shared group. I just didn't up date the trajectory subsetter tests to use a tempdir.

Copy link
Member Author

Choose a reason for hiding this comment

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

I could be convinced to move this to shared, but not for this PR. And I'd need a decent reason probably, because I'd bet they're already different. just looked, they are different, this one has the wrong comment on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I fixed the comment though: e955ef7

CHANGELOG.md Show resolved Hide resolved
@@ -402,7 +407,7 @@
"name": "python",
"nbconvert_exporter": "python",
"pygments_lexer": "ipython3",
"version": "3.10.12"
"version": "3.11.5"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here

Copy link
Member Author

Choose a reason for hiding this comment

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

same answer, just metadata, the version of python used when the notebook was edited.

Copy link
Member

@owenlittlejohns owenlittlejohns left a comment

Choose a reason for hiding this comment

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

I think this looks great.

It's a shame about the limitations on conditional logic in a Dockerfile. If that was possible, I'd probably recommend just adding something there to install the packages for these utilities when they are to be included.

I reckon the remove_results_files could be moved into the shared utilities (maybe with an optional argument or two, such as a directory to clean up, or a file extension), but I'm not going to hold the PR up over that.



def remove_results_files() -> None:
"""Remove all NetCDF-4 files downloaded during the Swath Projector
Copy link
Member

@owenlittlejohns owenlittlejohns Oct 15, 2024

Choose a reason for hiding this comment

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

There is something similar in the #95 and, from the documentation string here, I'm going to go out on a limb and suggest maybe there is a similar function in the Swath Projector test utilities.

@flamingbear
Copy link
Member Author

I reckon the remove_results_files could be moved into the shared utilities (maybe with an optional argument or two, such as a directory to clean up, or a file extension), but I'm not going to hold the PR up over that.

I prefer using a temporaryDirectory, so I didn't move it into the shared group. I just didn't up date the trajectory subsetter tests to use a tempdir.

@flamingbear flamingbear merged commit e23ee4b into main Oct 15, 2024
1 check passed
@flamingbear flamingbear deleted the mhs/DAS-none/fix-readme branch October 15, 2024 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants