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

save_hdfeos5.py: use mintpy.subset for naming if attributes missing #938

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

falkamelung
Copy link
Contributor

@falkamelung falkamelung commented Dec 27, 2022

Description of proposed changes
save_hdf5eos.py occasionally fails for subsets when attributes about the subset are missing because the subset information is used for the filename creation. With this PR, instead of failing, it uses the mintpy.subset information to generate the file name.

This is needed for MiaplPy because it uses SLCs which miss some attributes. I believe I have seen this error also for running mintpy only, but I forgot details.

Reminders

  • Fix #xxxx
  • Pass Pre-commit check (green)
  • Pass Codacy code review (green)
  • Pass Circle CI test (green)
  • Make sure that your code follows our style. Use the other functions/files as a basis.
  • If modifying functionality, describe changes to function behavior and arguments in a comment below the function declaration.
  • If adding new functionality, add a detailed description to the documentation and/or an example.

@yunjunz yunjunz self-requested a review December 28, 2022 03:20
geom_meta = readfile.read_attribute(geom_file) # FA 12/22: for TSX meta does not have relative_orbit attribute
unavco_meta['relative_orbit'] = int(geom_meta['unavco.relative_orbit'])
except ValueError:
print('Missing required attribute: relative_orbit')
Copy link
Member

@yunjunz yunjunz Dec 28, 2022

Choose a reason for hiding this comment

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

I don't see the motivation of this change here: time-series file is supposed to have more metadata than geometry files, always. Isn't that the case for your dataset?

To ensure we are on the same page on the details: the unavco.relative_orbit should had been translated into relative_orbit here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be that this is required for MiaplPy and that load_slc_geometry.py of MiaplPy does not use this MintPy function. relative_orbit in slcStack.h5 is currently none as you can see below. Maybe we should make a note how how this could be improved.

MiamiTsxSMD14/miaplpy/inputs[1014] info.py slcStack.h5 
******************** Basic File Info ************************
file name: /scratch/05861/tg851601/MiamiTsxSMD14/miaplpy/inputs/slcStack.h5
file type: timeseries
coordinates : RADAR

******************** Date Stat Info *************************
Start Date: 20170923
End   Date: 20210618
Number of dates  : 100
STD of datetimes : 1.08 years

******************** HDF5 File Structure ********************
Attributes in / level:
  ALOOKS                   1
  ANTENNA_SIDE             -1
  AZIMUTH_PIXEL_SIZE       2.1495932845300736
  BANDS                    1
  CENTER_LINE_UTC          40691.0
  DATA_TYPE                complex64
  DATE                     20200529
  EARTH_RADIUS             6348107.616403343
  FILE_LENGTH              7046
  FILE_PATH                /scratch/05861/tg851601/MiamiTsxSMD14/merged/SLC/20200529/20200529.slc
  FILE_TYPE                timeseries
  HEADING                  -168.4374330199127
  HEIGHT                   511035.8120019732
  INTERLEAVE               BIP
  LAT_REF1                 25.921521074257434
  LAT_REF2                 25.94302544748516
  LAT_REF3                 25.797651852239053
  LAT_REF4                 25.819170744512242
  LENGTH                   7046
  LON_REF1                 -80.02806403183072
  LON_REF2                 -80.16693071534473
  LON_REF3                 -80.05174311730326
  LON_REF4                 -80.19047876057562
  NCORRLOOKS               0.8178098213569981
  PLATFORM                 Tsx
  POLARIZATION             HH
  PRF                      3577.0218098958335
  PROCESSOR                isce
  PROJECT_NAME             MiamiTsxSMD14
  P_BASELINE_BOTTOM_HDR    -460.76757280546883
  P_BASELINE_TOP_HDR       -460.0380850366407
  RANGE_PIXEL_SIZE         1.3641053359701238
  RLOOKS                   1
  STARTING_RANGE           681478.0615665995
  SUBSET_XMAX              7168
  SUBSET_XMIN              0
  SUBSET_YMAX              7046
  SUBSET_YMIN              0
  UNIT                     i
  WAVELENGTH               0.03106658115133631
  WIDTH                    7168
  XMAX                     7167
  YMAX                     7045
  access_mode              read
  altitude                 511035.8120019732
  azimuthPixelSize         2.1495932845300736
  azimuthResolution        2.392
  beam_mode                SM
  byte_order               l
  data_type                complex64
  description              ['Single look complex image.']
  earthRadius              6348107.616403343
  extra_file_name          /scratch/05861/tg851601/MiamiTsxSMD14/merged/SLC/20170923/20170923.slc.vrt
  family                   slcimage
  file_name                /scratch/05861/tg851601/MiamiTsxSMD14/merged/SLC/20170923/20170923.slc
  image_type               slc
  isce_version             Release: 2.6.1, svn-, 20220811. Current: svn-.
  length                   7046
  name                     slcimage_name
  number_bands             1
  orbitNumber              56649
  polarization             HH
  prf                      3577.0218098958335
  radarWavelength          0.03106658115133631
  rangePixelSize           1.3641053359701238
  rangeResolution          1.49896229
  relative_orbit           None
  satelliteSpeed           7689.142061169693
  scheme                   BIP
  startUTC                 2017-09-01 11:18:06.651931
  startingRange            681478.0615665995
  stopUTC                  2017-09-01 11:18:17.196737
  trackNumber              None
  width                    7168
  xmax                     7168
  xmin                     0

HDF5 dataset "/bperp               ": shape=(100,)              , dtype=float32   , compression=None
HDF5 dataset "/date                ": shape=(100,)              , dtype=|S8       , compression=None
HDF5 dataset "/slc                 ": shape=(100, 7046, 7168)   , dtype=complex64 , compression=None
  MODIFICATION_TIME    1672180816.6942618

Copy link
Member

Choose a reason for hiding this comment

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

I don't like this change because it makes the code messy. I suggest grabbing the valid metadata, such as relative_orbit, from the geometry file here, if that has more metadata than the time-series file.

IMO, both files should have these types of metadata consistent in the miaplpy stage.

lat0 = lat1 + float(metadata['Y_STEP']) * int(metadata['LENGTH'])
lon1 = lon0 + float(metadata['X_STEP']) * int(metadata['WIDTH'])
elif 'mintpy.subset.lalo' in template.keys():
# for MiaplPy it would be preferrd to use miaplpy.subset.lalo but that is not available
Copy link
Member

Choose a reason for hiding this comment

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

I agree. Then let's modify the save_hdfeos5.py -t option to 1) be able to take multiple template files, 2) read the miaplpy template file and pass it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean adding -t for use outside smallbaselineApp with MiaplPy? That would make sense. But I think this is not only a MiaplPy issue. I believe I saw it occasionally for MintPy as well.

My preference would be to accept as is but make a note in the code that I will implement the -t option when I get a chance. After that we can switch back and we will see whether it really works.

The big question is what to do with Sara's geolocation correction and how to save the corrected coordinates in the S1 file. It would be good to implement this into MintPy but I don't see that Sara will have time for that any time soon. Unless you want to do it I probably will write a minsar script that would use save_hdfeos5.py -t.

Copy link
Member

Choose a reason for hiding this comment

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

I meant to modify save_hdfeos5.py to read both template files from mintpy and miaplpy, so that we could use miaplpy.subset.lalo as you wrote in the code comment.

My preference would be to accept as is but make a note in the code that I will implement the -t option when I get a chance. After that we can switch back and we will see whether it really works.

Sounds fine to me.

if suffix:
outName = fbase.removesuffix('_' + suffix) + SUB + '_' + suffix + fext
else:
outName = fbase + SUB + fext
Copy link
Member

Choose a reason for hiding this comment

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

Given the suffix is used here, I don't see the necessity of this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you referring to removesuffix? Well, it was needed. Otherwise I would not have added it. fbase seems to have occasionally an _.
You could make a mote to the code that this might be unnecessary and I can revisit next time I deal with this.

Copy link
Member

Choose a reason for hiding this comment

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

Please provide enough detailed examples, so that I could test this change. Otherwise, we can not accept the PR here.

@yunjunz
Copy link
Member

yunjunz commented Dec 28, 2022

Thanks @falkamelung. The general motivation sounds reasonable to me. I have a few comments on the implementation details.

Copy link
Contributor Author

@falkamelung falkamelung left a comment

Choose a reason for hiding this comment

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

Not sure what I have to say in this box. I did these changes weeks ago and in the next weeks it may be difficult to find the time to thoroghly test additional modifications.

I don't think anybody is using HDF5EOS anyway???

if suffix:
outName = fbase.removesuffix('_' + suffix) + SUB + '_' + suffix + fext
else:
outName = fbase + SUB + fext
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you referring to removesuffix? Well, it was needed. Otherwise I would not have added it. fbase seems to have occasionally an _.
You could make a mote to the code that this might be unnecessary and I can revisit next time I deal with this.

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.

None yet

2 participants