UTM Conversion Utility #321
Conversation
|
@dthomas1953 Doesn't the |
|
@nick-iris , I took a look at 'ph5.core.cs2cs.py`. There are a couple of show-stopping problems with trying to use this module to address #301.
The error is in this line: But even with that fixed, there is no way to enter easting/northing/etc. The Fairfield Nodes do not use (X,Y,Z) coordinates. |
|
Found some flake8 problems with entry_points, will clobber those and do a new pull request. |
|
@dthomas1953 Did you mean to close this pull request? We still need to review |
|
You can just push your flake8 changes to this PR |
|
Okay, stand by. |
|
flacated entry_points, updated CHANGELOG.TXT, next stop, review? |
ph5/utilities/utmtolatlong.py
Outdated
| PROG_VERSION = '2019.212' | ||
|
|
||
|
|
||
| class utm_conversions: |
There was a problem hiding this comment.
Class names should always be upper case. I suggest changing this class name to UTMConversions
ph5/utilities/utmtolatlong.py
Outdated
| return | ||
| hemisphere = str(args.side).upper() | ||
| if hemisphere != "S" and hemisphere != "N": | ||
| print ("Error, zone must be either N or S for northern/southern \ |
There was a problem hiding this comment.
You should be using a the logger, not printing to stdout.
ph5/utilities/utmtolatlong.py
Outdated
|
|
||
| def main(): | ||
|
|
||
| desc = 'Convert UTM locations to longitudes, latitudes, \ |
There was a problem hiding this comment.
I'm noticing these \^M carriage-return characters. Are you working on a Windows machine?
Please set the file encoding to UTF-8 and line-endings for new files to Unix in your IDE. This is so that text files are saved in a format that is not specific to the Windows OS and most easily shared across heterogeneous developer desktops.
ph5/utilities/utmtolatlong.py
Outdated
| parser.add_argument("-s", "--side", help="Side=Hemisphere, N or S") | ||
|
|
||
| args = parser.parse_args() | ||
| # print ("args:", args) |
There was a problem hiding this comment.
Remove commented out code
ph5/utilities/utmtolatlong.py
Outdated
| try: | ||
| fli = len(args.infile) | ||
| flo = len(args.outfile) | ||
| except (RuntimeError, TypeError, NameError, ValueError): |
There was a problem hiding this comment.
Why is the try/except necessary?
There was a problem hiding this comment.
I will get the class names nd comments cleaned up, and will set encoding to UTF-8. I'm using Gedit for Mac, I didn't see the ^M characters.
The try/except is necessary if the user uses the inline -e Easting -n Northing etc. method, which is distinct from the batch file method. In the inline case, the variables for infile and outfile are never defined, and testing them for length raises an error. They are only defined if user uses the -i ifile -o outfile option. Thus the try/catch.
Going to a funeral, will do another push later this morning.
There was a problem hiding this comment.
In the inline case, the variables for infile and outfile are never defined, and testing them for length raises an error. They are only defined if user uses the -i ifile -o outfile option.
Instead of catching these errors you should explicitly check if args.infile and args.outfile exist. If they exist then take their length, else set fli and flo to zero.
There was a problem hiding this comment.
I'm using Gedit for Mac, I didn't see the ^M characters.
You can see these carriage returns in GitHub (https://github.com/PIC-IRIS/PH5/pull/321/files).
|
@nick-iris , I added if easting < 0.0 or northing < 0.0:
warn = "Error, easting and northing must both be"
warn += " positive, but you used {0} and {1}"
LOGGER.info(warn.format(easting, northing))
returnThe program runs (doesn't crash), but no text is sent to the screen. How do I get a screen display like the following for command |
|
Just use the LOGGER for reporting errors or warnings. For example You can print the main output to stdout. I don't think you need the line with the |
|
Ran out of time today, we had to prep some field laptops and etc. Will bang out an update for Friday. |
|
@dthomas1953 There's no rush. Better to have everything be well written. |
There was a problem hiding this comment.
Nice work, and good job including tests.
There is some duplicate code, and some refactoring can be done to make this cleaner.
I think the structure to go for is:
class UTMConverter(object):
def __init__(self):
self.wgs = Proj(init="epsg:4326", proj="latlong")
def lat_long(self, northing, easting, zone, hemisphere):
# check sane inputs and raise errors or let lib raise errors from call
# convert...
return (lat, long)
def convert_file(infile, outfile):
utm = UTMConverter()
with open(infile) as infile, with open(outfile) as outfile:
for line in infile:
...
lat, long = utm.lat_long(northing=n....)
...
def print_convert(northing, ...):
utm = UTMConverter()
...
lat, long = utm.lat_long(northing=n....)
print(lat, long)
def main(argv):
import argparse
...
args = parser.parse_args(argv)
if file_way:
convert_files(...)
elif command_way:
print_convert(...) # or just do this inline here
if __name__ == '__main__':
import sys
main(sys.argv)This will allow you to test your args and parsing (prob not necessary for this)
test conversion alone
and test the file way with StringIO's
Some of the inline comments may not apply after this, or apply to the new location.
You can use argparser to be more specific about options, eliminating the try: fli = len...
infile doesn't need a -i switch. Can just be the last arg.
the file format should have a short description, example and where the format comes from.
The Proj() api is the string based clone of the proj command line. Is there a pythonic interface. If so that's the way to go
|
|
||
| def test_is_valid_utm_conversion(self): | ||
| u = utm_conversions(322916, 3771967, 13, 'N') # PIC Socorro | ||
| self.assertEqual(u.utm_to_latlong(), |
There was a problem hiding this comment.
Due to the inaccuracy and non-reproducibility of floating-point operations us assertAlmostEqual() when comparing floats.
Goes for all assertEqual() on floats.
| self.assertEqual(u.utm_to_latlong(), | ||
| (-106.91909595147378, 34.07349577107704)) | ||
|
|
||
| def test_is_invalid_utm_conversion(self): |
There was a problem hiding this comment.
What are you testing for here?
As you are calling the same function and args in the previous test, this will always fail if the other passes.
ph5/utilities/utmtolatlong.py
Outdated
|
|
||
| class utm_conversions: | ||
|
|
||
| def __init__(self, e=0, n=0, z=1, s='N'): |
There was a problem hiding this comment.
Use descriptive variable names, especially for call signatures.
ph5/utilities/utmtolatlong.py
Outdated
| return (lon, lat) | ||
|
|
||
| def dofromfile(self, args): | ||
| dater = open(args.infile) |
ph5/utilities/utmtolatlong.py
Outdated
| PROG_VERSION = '2019.212' | ||
|
|
||
|
|
||
| class utm_conversions: |
There was a problem hiding this comment.
Inherit from object for python2/3 consistency
ph5/utilities/utmtolatlong.py
Outdated
| @@ -0,0 +1,167 @@ | |||
| # This script can be used to convert UTM coordinates to Latitude/Longitude | |||
There was a problem hiding this comment.
Use tripple quotes for multiline comments.
There was a problem hiding this comment.
Good suggestions, I'll ponder these next week.
|
@ascire-pic, what file formats are you using or do you envision using?
What do you mean by integrated? |
|
Line 981 in 894357e Here is the code that currently does the conversion. Then an entry point CLI can wrap it and work on the file format / use case @ascire-pic needs. |
|
@dsentinel I needed to update the coordinates for a node PH5 where the array tables were automatically generated by segdtoph5. However, the PI gave me the updated locations in UTM instead of lat/lon because that is what the node server uses. To update the coordinates, I used keftoph5 and keftocsv to get a csv-format version of the array tables and then replaced the lat/lon columns with the corrected locations. To convert from UTM given to me to lat/lon I found a random converter online and used that, but it would have been easier if there was an easy-to-use UTM converter included in the PH5 software suite. I envisioned either feeding in a text file with columns for easting, northing and UTM zone, or having UTM as an input option in noven. |
|
@dsentinel , @nick-iris : Did the refactoring per Lloyd, ran flake8 on new files, tests pass, I think this is ready to go. I could use advice on "failing check" above. |
|
@dthomas1953 the test is failing during ph5/core/tests/test_ph5api.py It is failing the first time it tries to read lat and lon from a Node in the generated ph5 archive. It expects the ['location/X/value_d'] to be -105.405489539 and ['location/Y/value_d'] to be 47.6790599342 when it reads the array table for this node(id_s=500), but they are now both being written as 0.0 Side note, that test should also be changed to assertAlmostEqual. So I think something is now wrong with segd2ph5 when it tries to convert utm to lat, lon and write it in the array tables. |
|
Good clue, @derick-hess , I'll check it out. |
|
Hey, is there a way to capture test results to a file? When I try I was able to ctrl-break out of the tests and peruse the problem. It might be that, for hemisphere (north/south), the original method had it go to north if undefined, and I had changed it to None for the new way. Trying that fix now. |
|
The script that creates the PH5 test data set runs
So it should be setting the hemisphere correctly. It was before, and segdtoph5 was properly detecting north and south and handling that. You can just run an individual test script directly if you want. Also test_ph5api.py is going to continue to fail with Traceback (most recent call last):
File "/home/travis/build/PIC-IRIS/PH5/ph5/core/tests/test_ph5api.py", line 130, in test_array_t
self.assertEqual(channel[0]['location/X/value_d'], -105.405489539)
AssertionError: 0.0 != -105.405489539because segdtoph5 created incorrect entries in the array table. test_ph5api.py is reading the metadata and data from the created test PH5 data set. |
|
I'm running just the TestPH5API test, that reproduces the error. Unfortunately, the hemisphere of north vs None didn't help. Tracking it down! output: |
|
Just in case, make sure you deleted the old ph5 test data set and recreated it with create_ph5.sh before rerunning the test so it generates a new ph5 archive with the code changes |
|
@derick-hess Thanks, I hadn't done that! |
Doesn't / shouldn't the test create it. Many of the tests print that it will build it if not found. However, I don't think they all work. |
It can be done that way. The tests that are testing actually building PH5 do build a small data set and then delete is afterwards. obspytoPH5 and metadatatoph5 do this, as should tests for anythingtoph5 when those tests are written. You could have test_ph5api, test_ph5availability.py, test_ph5toms.py, test_ph5tostationxml.py and all the others build the ph5 in their setup and tear it back down. That would add a significant amount of time to running tests though. Also thought about just putting a test ph5 archive in a data directory, but didn't want to add a bunch of data files to the repository, but that is also an option. |
|
@nick-iris , @dsentinel this is finally ready to be merged. Has some UTM utilities (conversions in both directions) that the data group has been waiting for. |
|
When I create and run tests on my repository, the results are vastly different than shown here in Travis. Why? |
|
@dthomas1953 , You had a conflict merging from master. I resolved it for ya. |
|
@dsentinel thanks, I'll have a look. |
|
@dsentinel , @nick-iris I found the fly in the ointment, this should be OK to merge finally. |
environment.yml
Outdated
| @@ -11,7 +11,7 @@ dependencies: | |||
| - nose | |||
| - numpy | |||
| - numexpr | |||
| - pyproj | |||
| - pyproj=2.2 | |||
There was a problem hiding this comment.
What is the justification for pinning this?
ph5/core/ph5utils.py
Outdated
| @@ -68,6 +70,82 @@ def get_n_i(self, sensor_keys, datalogger_keys): | |||
| return ph5_resp.n_i | |||
|
|
|||
|
|
|||
| class UTMConversions: | |||
|
|
|||
| def __init__(self, lat, lon, side, zone): | |||
There was a problem hiding this comment.
I was thinking that one UTMConversions can be reused for a set of lat lons, but here a new one has to be created for each conversion?
ph5/core/ph5utils.py
Outdated
| @@ -68,6 +70,82 @@ def get_n_i(self, sensor_keys, datalogger_keys): | |||
| return ph5_resp.n_i | |||
|
|
|||
|
|
|||
| class UTMConversions: | |||
There was a problem hiding this comment.
Use singular from, drop the s
ph5/core/ph5utils.py
Outdated
| lon, lat = transformer.transform(float(easting), float(northing)) | ||
| return (lat, lon) | ||
|
|
||
| def geod2utm(self, lat, lon, elev): # utility function |
There was a problem hiding this comment.
Please avoid inline comments, and comments that don't add information.
ph5/core/ph5utils.py
Outdated
| return (lon, lat) | ||
|
|
||
|
|
||
| def run_geod(lat0, lon0, lat1, lon1, scalar=1.0): |
There was a problem hiding this comment.
Better named latlong2geod?
ph5/core/segyfactory.py
Outdated
| lat = self.array_t['location/Y/value_d'] | ||
| lon = self.array_t['location/X/value_d'] | ||
| elev = self.array_t['location/Z/value_d'] | ||
| utmc = ph5utils.UTMConversions(lat, lon, None, None) |
There was a problem hiding this comment.
...as above, shouldn't need to create a new instance for each conversion
ph5/core/segyfactory.py
Outdated
| lat = self.event_t['location/Y/value_d'] | ||
| lon = self.event_t['location/X/value_d'] | ||
| elev = self.event_t['location/Z/value_d'] | ||
| utmc = ph5utils.UTMConversions(lat, lon, None, None) |
ph5/utilities/novenqc.py
Outdated
| @@ -12,12 +12,12 @@ | |||
| from random import randint | |||
| from inspect import stack | |||
| from ast import literal_eval | |||
| from pyproj import Geod | |||
| from ph5.core import ph5utils # for run_geod | |||
| @@ -54,7 +54,7 @@ | |||
| 'nose', | |||
| 'numpy', | |||
| 'numexpr', | |||
| 'pyproj', | |||
| 'pyproj>=2', | |||
There was a problem hiding this comment.
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
ph5/core/ph5utils.py
Outdated
| @@ -68,6 +70,82 @@ def get_n_i(self, sensor_keys, datalogger_keys): | |||
| return ph5_resp.n_i | |||
|
|
|||
|
|
|||
| class UTMConversions: | |||
|
|
|||
| def __init__(self, lat, lon, side, zone): | |||
There was a problem hiding this comment.
side and zone can be None so they should probably be optional and default to None
Lat and long should not be required, only provided in the call of the functions latlong2XXX(lat, long)
e.g. review comment aug 2
|
@dsentinel , UTM class removed, loops simplified, comments cleaned up, functions renamed, I think it's ready this time. |
ph5/core/ph5utils.py
Outdated
|
|
||
| az, baz, dist = g.inv(lon0, lat0, lon1, lat1) | ||
|
|
||
| if dist: |
There was a problem hiding this comment.
If this conditional is to test ==0, then if dist == 0
| @@ -54,7 +54,7 @@ | |||
| 'nose', | |||
| 'numpy', | |||
| 'numexpr', | |||
| 'pyproj', | |||
| 'pyproj>=2', | |||
|
@dsentinel , I will tend to these. I took the I plan to go ahead and add a function to do the UPS (Univ. Polar Stereographic) conversions also, since I have the formula from Fairfield, and since we're expecting south pole data soon. |
|
@dsentinel @nick-falco I've added UPS (Univ. Polar Stereographic) support, cleaned up function names, replaced print commands with logger commands, and the other requested changes. Hoping this is a go for merging this time. |
There was a problem hiding this comment.
This still needs work...
I'm not sure how the tests are passing: Using Transformer.from_crs(...always_xy=False).transform() takes arguments and returns in order lat, long and northing, easting
previous comments and the proj version pinning are unresolved.
Please review your own code thoroughly before asking others :/
| return (northing, easting, utm_zone, hemisphere) | ||
|
|
||
|
|
||
| def lat_lon_elev_to_utm(lat, lon, elev): |
There was a problem hiding this comment.
This function is unnecessary.
There was a problem hiding this comment.
@dsentinel , Earlier you said consistent with ordering of arguments and return tuples i.e. lat, long and northing, easting
I endeavored to order the arguments consistently, and now have uniform calling of Y first (latitude, northing), X second (longitude easting), and that's why the calls now include always_xy = False. These functions are now working correctly, both for new functions (utm, ups) and for existing functions that needed utm conversions.
I don't understand what you mean by
I'm not sure how the tests are passing: Using Transformer.from_crs(...always_xy=False).transform() takes arguments and returns in order lat, long and northing, easting, can you explain what the issue is here?
I will remove the un-necessary function, rename the TSPC function, etc.
I am not pinning pyproj version at this time, that was a temporary occurrence that is not in the current version. It's resolved already. :/
| always_xy=False) | ||
| easting, northing = transformer.transform(lat, lon) | ||
| return (northing, easting, utm_zone, hemisphere) | ||
|
|
There was a problem hiding this comment.
Only a single blank line between functions.
| return (lat, lon) | ||
|
|
||
|
|
||
| def tspc_lat_lon(northing, easting): |
There was a problem hiding this comment.
Consistent function names ...to...
| epsgroot = "327" | ||
| elif hemisphere == "N": | ||
| epsgroot = "326" | ||
| epsg_utm = "EPSG:" + epsgroot + str(zone) |
There was a problem hiding this comment.
"EPSG:{}{}".format(epsgroot, zone)
There was a problem hiding this comment.
Looks good, will implement this syntax.
| transformer = Transformer.from_crs(epsg_wgs84, | ||
| epsg_utm, | ||
| always_xy=False) | ||
| easting, northing = transformer.transform(lat, lon) |
| @@ -54,7 +54,7 @@ | |||
| 'nose', | |||
| 'numpy', | |||
| 'numexpr', | |||
| 'pyproj', | |||
| 'pyproj>=2', | |||
There was a problem hiding this comment.
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
|
@dthomas1953 |
What does this PR do?
It adds capability to convert UTM coordinates to latitudes and longitudes. The relevant script is ph5:utilities:utmtolatlong.py, and has been integrated with entry_points.py, with the INGESTION group.
There are two ways to use it: interactive:
and Batch Mode. For the file utmdata.txt:
running the command
utmtolatlong -i utmdata.txt -o mylatlong.txtcreates the output file mylatlong.txt:
Relevant Issues?
"read UTM or UTM to lat/lon converter" #301
Checklist
CHANGELOG.txt. oops, will have to do that.