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

Switch to using reference/secondary for insar_isce_burst_job #291

Draft
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

forrestfwilliams
Copy link
Contributor

Should be released after the updates to HyP3. Notably, I included no validation of the reference/secondary age order because HyP3 will be checking this for us. Happy to hear feedback that we want this though.

@forrestfwilliams forrestfwilliams requested a review from a team August 21, 2024 12:45
@@ -424,16 +424,21 @@ def prepare_insar_job(cls,
return job_dict

def submit_insar_isce_burst_job(self,
granule1: str,
granule2: str,
*args,
Copy link
Contributor

@jhkennedy jhkennedy Aug 21, 2024

Choose a reason for hiding this comment

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

I think you can be a bit simpler here; if users are using positional arguments, they can just use reference and secondary since it's type hinted as Union[str, Iterable[str]] and should be functionally equivalent to using granule1, granule2. So we only need to maintain using granule1 and granule2 as keyword arguments, which we can do with this:

    def submit_insar_isce_burst_job(self,
                                    reference: Union[str, Iterable[str]] = None,
                                    secondary: Union[str, Iterable[str]] = None,
                                    name: Optional[str] = None,
                                    apply_water_mask: bool = False,
                                    looks: Literal['20x4', '10x2', '5x1'] = '20x4',
                                    *,
                                    granule1: Optional[str] = None,
                                    granule2: Optional[str] = None,
                                    ) -> Batch:

Notably, this keeps the same number of positional arguments and keeps name, apply_water_mask and looks in the same position.

Then after the function definition, you would check granules and reference/secondary; something like:

if `reference` is None:
    if `granule1` is None:
        raise ValueError('Either reference and secondary must be provided, or granule1 and granule2')
    reference = granule1

if `secondary` is None:
    if `granule2` is None:
        raise ValueError('Either reference and secondary must be provided, or granule1 and granule2')
    secondary = granule2

and you can drop all of the arguments = locals().copy() stuff (there's no harm in a granule1 and granule2 variable existing).


Maybe not this PR, but in the future, it might be worth moving the *, before name, forcing the options to come in as keyword arguments.

arguments['reference'] = args[0]
arguments['secondary'] = args[1]

if arguments['granule1']:
Copy link
Contributor Author

@forrestfwilliams forrestfwilliams Aug 21, 2024

Choose a reason for hiding this comment

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

@jhkennedy I believe we're already doing what you're suggesting (at least in part) here (allowing people to use granules but raising a warning if they do. I like raising an error if neither are specified and will add that. Also correct that we're not breaking yet, just prepping to.

Copy link
Contributor

Choose a reason for hiding this comment

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

@forrestfwilliams you're breaking the interface here, which you do not need to do with my suggestions. Examples, 👇

Copy link
Contributor

@jhkennedy jhkennedy Aug 21, 2024

Choose a reason for hiding this comment

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

You're breaking the interface by not maintaining the same number and order of positional arguments. For example, this works with the current SDK and my suggestion but not with this PR:

job = hyp3.submit_insar_job('S1_...-BURST', 'S1_...-BURST`, 'my_job_name')

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll just open a PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

Co-authored-by: Andrew Player <[email protected]>
@forrestfwilliams
Copy link
Contributor Author

For some reason, mamba/conda cannot locate the build package even though it's on conda-forge. This is causing the tests to fail.

@forrestfwilliams
Copy link
Contributor Author

This PR is good-to-go from my perspective, but we should hold off on merging it for the time being. We need to wait to update HyP3 and the SDK until Disco can support the breaking change in Vertex.

@forrestfwilliams forrestfwilliams marked this pull request as draft August 23, 2024 13:22
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