PIC-IRIS / PH5

Library of PH5 clients, apis, and utilities
Other
15 stars 9 forks source link

[BUG] ph5tostationxml regression #410

Closed jschaeff closed 3 years ago

jschaeff commented 4 years ago

Describe the bug With ph5tostationxml installed from commit 5c60ad8c74e7d27802964c41a4d5771d8befbfc5 the script works fine. With ph5tostationxml from commit f43f12f052081f148536f02c7575914504ce2824 the script complains for every channel:

[2020-06-29 14:51:29,653] - ph5.clients.ph5tostationxml - WARNING: array 002, station 95, channel 2: Channel longitude seems to be 0. Is this correct???

Environment (please complete the following information):

To Reproduce Steps to reproduce the behavior: On the cloned source repository, run something like:

git chekout f43f12f052081f148536f02c7575914504ce2824
/opt/miniconda3/envs/ph5/bin/ph5tostationxml -n master.ph5 -p my_PH5_archive --level network --format text
git checkout 5c60ad8c74e7d27802964c41a4d5771d8befbfc5
/opt/miniconda3/envs/ph5/bin/ph5tostationxml -n master.ph5 -p my_PH5_archive --level network --format text

Expected behavior the latest ph5tostationxml should behave like the old one. Futhermore, with the new ph5tostationxml you need to press a key to continue, which is a pain for scripting:

[2020-06-29 14:51:29,654] - ph5.clients.ph5tostationxml - WARNING: array 002, station 99, channel 3: Channel longitude seems to be 0. Is this correct???
Check logs for errors. Please keep in mind that the entries with errors will not be displayed in the result.
Hit Enter/Return to continue.
#Network|Description|StartTime|EndTime|TotalStations
ZO|Argentieres|2018-04-24T12:50:53|2018-06-06T12:00:01|98
dsentinel commented 4 years ago

Thanks for the report @jschaeff , Error and warning reporting has been worked on, and has hopefully only improved. This is only a warning, and is useful to let users know that they may not have locations for channels. Does this disrupt your workflow?

@damhuonglan, why "Hit Enter... to continue"? I don't think we want this, should probably revert.

damhuonglan commented 4 years ago

Thanks for the report @jschaeff , Error and warning reporting has been worked on, and has hopefully only improved. This is only a warning, and is useful to let users know that they may not have locations for channels. Does this disrupt your workflow?

@damhuonglan, why "Hit Enter... to continue"? I don't think we want this, should probably revert.

I can revert. The reason I did that because xml is too long, and I need to scroll up to see the errors and warning list before that.

jschaeff commented 4 years ago

Thanks for the quick response. @dsentinel yes the warning is an improvement, I understand now. The PH5 archive has locations on the stations, but not on the channels, I guess. I'll check this with my coworker.

timronan commented 4 years ago

Why is it disallowed for a channel to have a longitude=0? Zero is a real longitude on the Earth's surface and both stations and channels should have latitude and longitude information even if it required to be identical. See issue #402.

Could it be possible to compile a report of the warnings after the script is run. Or exit on a warning. Hitting enter for every channel that throws a warning seems arduous especially for large expirments with lots of nodes.

damhuonglan commented 4 years ago

Why is it disallowed for a channel to have a longitude=0?

This happens because I combine validation in ph5validate and ph5tostationxml. The code in ph5validate checked latitude and longitude=0 and ask user about it. I will check with DataGroup about this to decide about reverting. Thanks for your comment.

dsentinel commented 4 years ago

The pause was a mistake and is to be removed.

Perhaps the warning is too and can be removed. It's not disallowed, it just warns, probably because it's likely this is the default and never set.

hrotman-pic commented 3 years ago

It looks like this was resolved by #412 and #435 .