-
Notifications
You must be signed in to change notification settings - Fork 7
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
Issue94 mgrs tile code to lat lon utility #103
Conversation
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.
Branch looks good, had some minor findings and suggestions for improvements, but I'm most concerned about making sure the unit test runs from within the container. See my comment on the skipIf
decorator line.
|
||
@skipIf(not mgrs_osr_gdal_are_available(), reason="gdal, osr, and/or mgrs is not installed on the local instance") | ||
def test_get_geographic_boundaries_from_mgrs_tile(self): | ||
from opera.util.metadata_utils import get_geographic_boundaries_from_mgrs_tile |
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.
Please co-locate this with the rest of the imports at the top of the module
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.
I think I need more input on this. I moved this into the @skipIf scope because otherwise it will raise an exception in get_geographic_boundaries_from_mgrs_tile() when it tries to import osgeo if it doesn't exist. I don't think that we want to suppress the import exception in get_geographic_boundaries_from_mgrs_tile().
One thought is that I can add try/except to the import in test_metadata_utils.py:
try:
# If mgrs, osr, or gdal are not available for import, this will raise an exception
from opera.util.metadata_utils import get_geographic_boundaries_from_mgrs_tile
except:
pass
Am I missing a more elegant solution for this?
Here is the exception raised if moved out of the skipIf scope:
Traceback:
../../../.conda/envs/opera/lib/python3.8/importlib/__init__.py:127: in import_module
return _bootstrap._gcd_import(name[level:], package, level)
src/opera/test/util/test_metadata_utils.py:15: in <module>
from opera.util.metadata_utils import get_geographic_boundaries_from_mgrs_tile
src/opera/util/metadata_utils.py:13: in <module>
from osgeo import gdal
E ModuleNotFoundError: No module named 'osgeo'
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.
Good point, I left out a piece of the puzzle here, which is adding an import check in metadata_utils.py
to swap out the osr
module with a mock implementation of the osr
module if from osgeo import osr
fails (seems like gdal
is not getting used in metadata_utils.py
currently). However, I'm fine with deferring this to a later PR.
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.
Updates all look great, thanks for the quick turnaround!
Initial implementation of utility to convert from MGRS tile codes to lat/lon bounding box.