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

Update Tests for HyBIG #76

Merged
merged 6 commits into from
Oct 19, 2023
Merged

Update Tests for HyBIG #76

merged 6 commits into from
Oct 19, 2023

Conversation

flamingbear
Copy link
Member

Description

Updates the existing regression tests to work with the recent changes to HyBIG.
These include:

  • Tiling now only by number of grid cells.
  • Parameter validation
  • resolution defaults

Jira Issue ID

DAS-1969

Local Test Steps

Ensure latest HyBIG image is running in your Harmony in a box.

Pull this branch locally and build regression images, specifically hoss and hybig.

Edit the hoss and hybig notebooks to run against localstack and run each notebook against localstack.

PR Acceptance Checklist

  • Acceptance criteria met
  • Tests added/updated (if needed) and passing
  • Documentation updated (if needed)

This removes an aster test without any parameters because that causes massive
tiling.

It also updates the test for tiling due to the new cell count based tiling trigger.
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.

Nice work.

The main thing I found was that one test didn't pass for me locally. Otherwise I only had silly formatting nits.

test/hoss/HOSS_Regression.ipynb Show resolved Hide resolved
"id": "e8deff1f",
"metadata": {},
"source": [
"### Test: Scale Sizes are Positive\n"
Copy link
Member

Choose a reason for hiding this comment

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

I got a failure here (I tried it a couple of times):

HTTPError: 400 Client Error: Bad Request for url: http://localhost:3000/C1256584478-EEDTEST/ogc-api-coverages/1.0.0/collections/all/coverage/rangeset?forceAsync=true&outputcrs=EPSG%3A4326&scaleSize=0.002%2C-0.002&granuleId=G1256584570-EEDTEST&format=image%2Fjpeg

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting, what happens if you just visit that url in the failure?

http://localhost:3000/C1256584478-EEDTEST/ogc-api-coverages/1.0.0/collections/all/coverage/rangeset?forceAsync=true&outputcrs=EPSG%3A4326&scaleSize=0.002%2C-0.002&granuleId=G1256584570-EEDTEST&format=image%2Fjpeg

Because this particular test is actually failing in harmony, not HyBIG, but I left the test in anyway. This is related to the question, should Harmony have a validation check for scaleExtent? See comments on the ticket.

I have the latest and greatest harmony-in-a-box and this is what I see when I visit the URL:

Screenshot 2023-10-19 at 8 53 19 AM

Copy link
Member

Choose a reason for hiding this comment

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

I did see the comments in the ticket, yup. If this is testing Harmony-core functionality, it probably lives in one of the Harmony-core regression test notebooks.

I also have the latest Harmony-in-a-box. The URL fails when placed directly in the browser, with the error that you have in this comment. But I continue to get this other error when using harmony-py, which likely means that harmony-py is doing something additional here in validating the URL before it gets sent to Harmony, maybe. Do you see different behaviour when running the request via harmony-py?

Copy link
Member Author

Choose a reason for hiding this comment

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

Using harmony-py, like the test? It passes for me. Raises the expected error.

Copy link
Member

Choose a reason for hiding this comment

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

As discussed in Slack - differences here were due to my local environment having an older version of harmony-py. I get this test to pass now.

test/hybig/HyBIG_Regression.ipynb Show resolved Hide resolved
test/hybig/HyBIG_Regression.ipynb Outdated Show resolved Hide resolved
test/hybig/HyBIG_Regression.ipynb Outdated Show resolved Hide resolved
test/hybig/HyBIG_Regression.ipynb Show resolved Hide resolved
@flamingbear
Copy link
Member Author

silly formatting nits.

https://pypi.org/project/jupyter-black/

👀

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.

Thanks for fixing up the whitespace, and sorry for having a stale version of the harmony-py leading to some confusion.

I think it's a good question about the location of the scale extent ordering validation. My gut instinct is that is really something that lives in a Harmony-core test suite, but that is probably a broader effort, because there aren't really those sort of tests in either of the harmony or harmony-regression notebooks.

@flamingbear
Copy link
Member Author

I think it's a good question about the location of the scale extent ordering validation. My gut instinct is that is really something that lives in a Harmony-core test suite, but that is probably a broader effort, because there aren't really those sort of tests in either of the harmony or harmony-regression notebooks.

I am going to leave them here for now, but would like to change that if harmony ends up validating scale_extents.

@flamingbear flamingbear merged commit 31fb757 into main Oct 19, 2023
@flamingbear flamingbear deleted the mhs/DAS-1969/update-regressions branch October 19, 2023 17:24
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.

2 participants