Skip to content

HARMONY-2035: As a harmony-py user, I want to request pixel subsetting.#109

Merged
ygliuvt merged 4 commits into
mainfrom
harmony-2035
Mar 14, 2025
Merged

HARMONY-2035: As a harmony-py user, I want to request pixel subsetting.#109
ygliuvt merged 4 commits into
mainfrom
harmony-2035

Conversation

@ygliuvt

@ygliuvt ygliuvt commented Mar 12, 2025

Copy link
Copy Markdown
Member

Jira Issue ID

HARMONY-2035

Description

This PR is the harmony-py part to support client requesting pixel subsetting. There is a corresponding harmony PR that should be used to test the harmony-py change.
It adds a new pixel_subset parameter in harmony-py request to support pixel subsetting.

Local Test Steps

The pixel_subset parameter can be unset, True or False in a harmony-py request. Feel free to experiment with different values in the following tests.

Use the following block to test OGC Rangeset requests:

import helper
helper.install_project_and_dependencies('..', libs=['examples'])
import json
import sys
import datetime as dt
from harmony import BBox, Client, Collection, Request, Environment

harmony_client = Client(env=Environment.LOCAL)
collection = Collection(id='C1233800302-EEDTEST')
request = Request(
    collection=collection,
    spatial=BBox(-140, 20, -50, 60),
    granule_id='G1233800343-EEDTEST',
    crs='EPSG:31975',
    variables=['blue_var'],
    format='image/png',
    pixel_subset=True
)

url = harmony_client.request_as_url(request)
print(url)

job_id = harmony_client.submit(request)

for filename in [f.result() for f in harmony_client.download_all(job_id)]:
    print(f'\nDownload file: {filename}')

Use the following block to test OGC EDR requests:

import helper
helper.install_project_and_dependencies('..', libs=['examples'])
import json
import sys
import datetime as dt
from harmony import WKT, BBox, Client, Collection, Request, Environment

harmony_client = Client(env=Environment.LOCAL)
collection = Collection(id='C1233800302-EEDTEST')

request = Request(
    collection=collection,
    spatial=WKT('POLYGON((-140 20, -50 20, -50 60, -140 60, -140 20))'),
    granule_id=['C1233800302-EEDTEST'],
    max_results=1,
    temporal={
        'start': dt.datetime(1980, 1, 1),
        'stop': dt.datetime(2020, 12, 30)
    },
    variables=['blue_var'],
    crs='EPSG:31975',
    format='image/png',
    pixel_subset=False
)

url = harmony_client.request_as_url(request)
print(url)

job_id = harmony_client.submit(request)
for filename in [f.result() for f in harmony_client.download_all(job_id)]:
    print(f'\nDownload file: {filename}')

PR Acceptance Checklist

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

@ygliuvt ygliuvt mentioned this pull request Mar 12, 2025
Closed
4 tasks
chris-durbin
chris-durbin reviewed Mar 12, 2025
View reviewed changes

@chris-durbin chris-durbin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I found setting pixel_subset='Foo' was allowed and sent the request to harmony. I'd like if we enforced that all booleans must be booleans and that we set an error message indicating that if someone tries to set it to anything other than True or False (possibly None but we'd have to make sure that works correctly and does not add the parameter to the request).

Comment thread tests/test_request.py Outdated

def test_request_defaults_to_pixel_subset_none():
request = Request(collection=Collection('foobar'))
assert request.pixel_subset is None No newline at end of file

@chris-durbin chris-durbin Mar 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd like to test that setting pixel_subset to a non-boolean value causes request.is_valid() to be False and an appropriate error message is provided.

@ygliuvt ygliuvt Mar 13, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added boolean parameter validations.

Comment thread harmony/client.py
params['forceAsync'] = True
# use string in lowercase as value to match how boolean values
# in params are converted below
params['forceAsync'] = 'true'

@chris-durbin chris-durbin Mar 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd prefer if we used booleans throughout for all boolean parameters and treat it as an error if a string or any other type is used.

We can add the boolean validations to this function:

def error_messages(self) -> List[str]:
        """A list of error messages, if any, for the request."""

@ygliuvt ygliuvt Mar 13, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is used to submit to Harmony which has to be a string now.

chris-durbin
chris-durbin approved these changes Mar 14, 2025
View reviewed changes

@chris-durbin chris-durbin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tested successfully, just a comment on changing the validation error message.

Comment thread harmony/request.py Outdated
('Destination URL must be an S3 location'))
'Destination URL must be an S3 location'),
(self.concatenate is None or isinstance(self.concatenate, bool),
'concatenate must be either True of False'),

@chris-durbin chris-durbin Mar 14, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

'True of False' typo in several places.

Since we are requiring a boolean type (pixel_subset='True' fails validation), we should say something like:
'concatenate must be a boolean type either True or False'

@ygliuvt ygliuvt Mar 14, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Updated message to concatenate must be a boolean (True or False)

@ygliuvt ygliuvt merged commit 57b44f5 into main Mar 14, 2025
@ygliuvt ygliuvt deleted the harmony-2035 branch March 14, 2025 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@indiejames indiejames indiejames approved these changes

@chris-durbin chris-durbin chris-durbin approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ygliuvt @indiejames @chris-durbin