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

s1_geocode_stack.py: Add ability to read "DH" SAFE files #193

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

scottstanie
Copy link
Contributor

This adds the ability to recognize SAFE files that have HH/HV instead of VV/VH.
Right now I'm raising a NotImplementedError in _get_pol_str if you try to ask for dual-pol, since I'm not sure how we're handling that elsewhere in the code and it seems like it would need some further refactoring to allow.

Copy link
Contributor

@seongsujeong seongsujeong left a comment

Choose a reason for hiding this comment

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

Looks good overall. Couple of comments are in the below. - Seongsu

@@ -135,6 +139,30 @@ def generate_burst_map(zip_files, orbit_dir, output_epsg=None, bbox=None,
burst_map = pd.DataFrame(data=burst_map)
return burst_map

def _get_pol_str(zip_file, pol_type):
"""Get the polarization string from the zip file name and the type of polarization."""
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to mention what values are expected for pol_type in the docstring.

Comment on lines +152 to +165
if count == "S" and pol_type in ["dual-pol", "cross-pol"]:
raise ValueError(f"{zip_file} is single-pol. Unable to process {pol_type}")
if pol_type == "dual-pol":
raise NotImplementedError("TODO")
if h_or_v == "H":
return ["hh", "hv"]
else:
return ["vv", "vh"]
elif pol_type == "cross-pol":
return 'hv' if h_or_v == "H" else "vh"
elif pol_type == "co-pol":
return 'hh' if h_or_v == "H" else "vv"
else:
raise ValueError(f"Invalid {pol_type = }")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if count == "S" and pol_type in ["dual-pol", "cross-pol"]:
raise ValueError(f"{zip_file} is single-pol. Unable to process {pol_type}")
if pol_type == "dual-pol":
raise NotImplementedError("TODO")
if h_or_v == "H":
return ["hh", "hv"]
else:
return ["vv", "vh"]
elif pol_type == "cross-pol":
return 'hv' if h_or_v == "H" else "vh"
elif pol_type == "co-pol":
return 'hh' if h_or_v == "H" else "vv"
else:
raise ValueError(f"Invalid {pol_type = }")
if pol_type not in ["dual-pol", "cross-pol", "co-pol"]:
raise ValueError(f"Invalid {pol_type = }")
if count == "S" and pol_type in ["dual-pol", "cross-pol"]:
raise ValueError(f"{zip_file} is single-pol. Unable to process {pol_type}")
if pol_type == "dual-pol":
raise NotImplementedError("TODO")
if h_or_v == "H":
return ["hh", "hv"]
else:
return ["vv", "vh"]
if pol_type == "cross-pol":
return "hv" if h_or_v == "H" else "vh"
if pol_type == "co-pol":
return "hh" if h_or_v == "H" else "vv"

Early returns for readability

@seongsujeong
Copy link
Contributor

Another thought. Currently the default pol for load_bursts() in s1-reader is fixed to vv (here). I think this functionality would be useful for the s1-reader. So, what do you think about putting this function into s1-reader @scottstanie?

@scottstanie
Copy link
Contributor Author

Oh that's a great point. I knew there was somewhere that made more sense than tucked away in this stack script lol.

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.

2 participants