-
Notifications
You must be signed in to change notification settings - Fork 24
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
Remove isce dep #105
base: main
Are you sure you want to change the base?
Remove isce dep #105
Conversation
Reviewer's Guide by SourceryThis pull request removes the isce2 dependency for loading TOPS data. It also adds the number of looks to the snaphu command. Sequence diagram for TOPS metadata extraction processsequenceDiagram
participant C as Client
participant M as MetaUtils
participant X as XML Parser
participant U as Utils
C->>M: extract_tops_metadata(xml_file)
M->>X: parse XML file
X-->>M: return parsed data
M->>U: interpolateHermite(state_vectors)
U-->>M: return positions & velocities
M->>U: xyz_to_llh(positions)
U-->>M: return coordinates
M->>U: enubasis(coordinates)
U-->>M: return transformation matrices
M-->>C: return metadata & frame info
Class diagram showing metadata extraction changesclassDiagram
class MetaUtils {
+extract_tops_metadata(xml_file)
+extract_stripmap_metadata(xml_file)
+extract_alosStack_metadata(xml_file)
}
class AstronomicalHandbook {
+PlanetsData
+SatellitesData
+Constants
}
class Utils {
+xyz_to_llh()
+enubasis()
+radiusOfCurvature()
+interpolateHermite()
}
MetaUtils --> AstronomicalHandbook
MetaUtils --> Utils
note for MetaUtils "Replaces ISCE2 dependency"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @mirzaees - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding tests to verify the new TOPS metadata extraction produces identical results to the original ISCE2 version
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -364,6 +364,8 @@ def unwrap_parser(): | |||
help='Width of Reference .h5 file') | |||
parser.add_argument('-sl', '--length', dest='ref_length', type=int, default=None, | |||
help='Length of .h5 file') | |||
parser.add_argument('-n', '--nlooks', dest='nlooks', type=int, default=200, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (bug_risk): Add validation for the nlooks parameter to ensure it's within reasonable bounds.
SNAPHU's correlation looks parameter should be positive and reasonably sized. Consider adding bounds checking to prevent invalid values.
Suggested implementation:
def validate_nlooks(value):
try:
nlooks = int(value)
if nlooks <= 0:
raise argparse.ArgumentTypeError("nlooks must be positive")
if nlooks > 10000: # reasonable upper bound for SNAPHU
raise argparse.ArgumentTypeError("nlooks value is unreasonably large (>10000)")
return nlooks
except ValueError:
raise argparse.ArgumentTypeError("nlooks must be an integer")
parser.add_argument('-n', '--nlooks', dest='nlooks', type=validate_nlooks, default=200,
help='The equivalent number of independent looks (positive integer, max 10000)')
You'll need to ensure that argparse is imported at the top of the file. If it's not already there, add:
import argparse
This would remove isce2 dependency for tops data when loading them. There is still work to be done for stripmap
It also adds the correct number of looks for snaphu unwrapping
Summary by Sourcery
Remove ISCE dependency for TOPS data.
Enhancements:
meta_utils
instead ofisce_utils
for TOPS data metadata extraction.Tests: