opera-adt / COMPASS

COregistered Multi-temPorAl Sar Slc
Apache License 2.0
39 stars 18 forks source link

[New Feature]: Refactoring to have output filename generation be a method #57

Closed scottstanie closed 1 year ago

scottstanie commented 2 years ago

Checked for duplicates

Yes - I've already checked

Alternatives considered

No - I haven't considered

Related problems

Right now there are 5 locations where the output path is built up in an f-string from variables as {burst_id}_{date_str}_{pol}.slc

https://github.com/opera-adt/COMPASS/blob/45d325b4399e37c72d2d307eee8b3549c44a47f8/src/compass/s1_geocode_slc.py#L88-L89 https://github.com/opera-adt/COMPASS/blob/45d325b4399e37c72d2d307eee8b3549c44a47f8/src/compass/s1_rdr2geo.py#L72-L74

https://github.com/opera-adt/COMPASS/blob/45d325b4399e37c72d2d307eee8b3549c44a47f8/src/compass/s1_resample.py#L84-L90 https://github.com/opera-adt/COMPASS/blob/45d325b4399e37c72d2d307eee8b3549c44a47f8/src/compass/utils/geo_metadata.py#L101 https://github.com/opera-adt/COMPASS/blob/45d325b4399e37c72d2d307eee8b3549c44a47f8/src/compass/utils/range_split_spectrum.py#L90-L91

Besides needing to make sure these all match if we ever change them, there are now 5 places to change the output extension once we finish off the COG PR: https://github.com/opera-adt/COMPASS/pull/28

Describe the feature request

I'd like a single source of truth to make the output filename, which would make it easier to adjust to accomodate GTiff/HDF5/ENVI formats.

I'm picturing something like how the output path is made as a property of the GeoRunconfig: made by the GeoRunconfig object: https://github.com/opera-adt/COMPASS/blob/45d325b4399e37c72d2d307eee8b3549c44a47f8/src/compass/utils/geo_runconfig.py#L113-L117

Does it seems like a good idea to have another property to make an output string? It might have to go into the s1reader repo, but I was picturing something like

@property
def output_name(self):
    date_str = self.sensing_start.strftime("%Y%m%d")
    burst_id = self.burst_id
    pol = self.polarization
    return f'{burst_id}_{date_str}_{pol}'

Alternatively, it could just be a utility function in in this repository to take a burst and output that string:

# maybe in utils.helpers
def get_output_name(burst, ext=".tiff"):
    date_str = burst.sensing_start.strftime("%Y%m%d")
    burst_id = burst.burst_id
    pol = burst.polarization
    return f'{burst_id}_{date_str}_{pol}{ext}'
vbrancat commented 1 year ago

@LiangJYu have we address this? If yes, let's close this issue

LiangJYu commented 1 year ago

@LiangJYu have we address this? If yes, let's close this issue

This is partially addressed in geocode_slc with this but not so much elsewhere.

vbrancat commented 1 year ago

@LiangJYu I think we have addressed this entirely. Can you confirm?

LiangJYu commented 1 year ago

@LiangJYu I think we have addressed this entirely. Can you confirm?

It is not entirely addressed. I will fix and submit a PR.

LiangJYu commented 1 year ago

When #194 is merged, I think issue can be closed.

vbrancat commented 1 year ago

Closing as #194 has been merged!