search

eqcorrscan / EQcorrscan

Earthquake detection and analysis in Python.
https://eqcorrscan.readthedocs.io/en/latest/
Other
166 stars 86 forks source link

Shorten and clarify a warning message #485

Closed dr-glenn closed 2 years ago

dr-glenn commented 2 years ago

A warning message from multi_corr.c spanned multiple lines and was incorrectly terminated. The message has been shortened and made a bit more informative. The message had a newline in the wrong place.

PR Checklist

calum-chamberlain commented 2 years ago

Hi @dr-glenn - thanks for this - it looks like you made your changes on the wrong branch. Was the only change that you meant to make the changes to multi_corr.c? If so I can make those changes to develop in a new PR (or you could)?

dr-glenn commented 2 years ago

I would appreciate it if you would put that one change into the correct branch - I have too many ways to do this wrong. At sometime I was advised to submit changes to develop branch. But I probably cloned from master. I've never collaborated on any Git hosted project before, so it was easy to mess it up! Was I supposed to clone from develop?

I would like to understand the meaning of the countless cccsum messages and perhaps make them more helpful. I am very puzzled that the cccsum error messages appear as INFO messages! Here is the code in lag_calc.py: msg = ('lag-calc has decreased cccsum from %f to %f - ' % (detection.detect_val, checksum)) Logger.error(msg) In my output the message is labeled as "INFO". How does that happen?

BTW, my neighbor is a developer/manager at Github and I asked for his help. To my lament that git was perplexing, he said that no one truly understands it. Kind of like quantum mechanics. Oh great.

Glenn Nelson in Santa Cruz

On Sun, Mar 13, 2022 at 1:40 PM Calum Chamberlain @.***> wrote:

Hi @dr-glenn https://github.com/dr-glenn - thanks for this - it looks like you made your changes on the wrong branch. Was the only change that you meant to make the changes to multi_corr.c? If so I can make those changes to develop in a new PR (or you could)?

— Reply to this email directly, view it on GitHub https://github.com/eqcorrscan/EQcorrscan/pull/485#issuecomment-1066178184, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACHQDVIDXMZWG7I2ZSXM4HTU7ZHCTANCNFSM5QRANW6Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

dr-glenn commented 2 years ago

BTW, I am working with Susan Schwartz at UCSC. Got my Ph.D. in 1991, but then went into Silicon Valley software development. Retired now, so I've returned to a better place.

Glenn Nelson in Santa Cruz

On Sun, Mar 13, 2022 at 1:40 PM Calum Chamberlain @.***> wrote:

Hi @dr-glenn https://github.com/dr-glenn - thanks for this - it looks like you made your changes on the wrong branch. Was the only change that you meant to make the changes to multi_corr.c? If so I can make those changes to develop in a new PR (or you could)?

— Reply to this email directly, view it on GitHub https://github.com/eqcorrscan/EQcorrscan/pull/485#issuecomment-1066178184, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACHQDVIDXMZWG7I2ZSXM4HTU7ZHCTANCNFSM5QRANW6Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

calum-chamberlain commented 2 years ago

Hi Glenn, thanks I will close this PR and open a new one with that change based on develop.

when you clone, you clone the whole repository (all of the branches), and you should either have checked out develop and made changes there, then made a pull request to develop, OR made changes on master and made a pull request to master. The way EQcorrscan does its branching is to maintain master as a current-version branch, and all changes are made to develop then merged to master before a new release (apart from documentation changes which are made directly to master so that the docs get updated immediately). Apologies, it isn't a great model! Nor is it well described.

Error messages (Logger.error) should never appear as INFO type. Are you sure that line is the source of your INFO messages?

dr-glenn commented 2 years ago

I see many messages like this: 2022-03-14 12:57:35,549 - main - INFO: lag-calc has decreased cccsum from 0.668884 to 0.379192 -

I wanted to know where they came from and perhaps understand what they meant. I am sure that it comes from lag_calc.py, vicinity of line 326.

Glenn Nelson in Santa Cruz

On Mon, Mar 14, 2022 at 1:15 PM Calum Chamberlain @.***> wrote:

Hi Glenn, thanks I will close this PR and open a new one with that change based on develop.

when you clone, you clone the whole repository (all of the branches), and you should either have checked out develop and made changes there, then made a pull request to develop, OR made changes on master and made a pull request to master. The way EQcorrscan does its branching is to maintain master as a current-version branch, and all changes are made to develop then merged to master before a new release (apart from documentation changes which are made directly to master so that the docs get updated immediately). Apologies, it isn't a great model! Nor is it well described.

Error messages (Logger.error) should never appear as INFO type. Are you sure that line is the source of your INFO messages?

— Reply to this email directly, view it on GitHub https://github.com/eqcorrscan/EQcorrscan/pull/485#issuecomment-1067244342, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACHQDVN6DZIHQ65X2ULMW33U76M6FANCNFSM5QRANW6Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

dr-glenn commented 2 years ago

Calum, I have another question and I don't see myself making a pull request until I know more. Templates have some strange name restrictions. For example here is one of mine: 2019_03_23t23_10_12.tgz The only special character allowed is underscore! And only lower case letters are allowed! Why on earth (or deep within it) are these limitations made? I wanted to use underscore and hyphen and capital 'T'. (I also discovered that using colon in a filename was very bad when I copied the file to Windows). I don't think the same restrictions are imposed on match_filter and lag_calc output files

BTW, I name my detection plots like this: Templ-2019-07-30T07-22-09_Det-2018-05-12T08-49-09.png. Very useful. I have programs that catalog the detections based on the image filenames.

Glenn Nelson in Santa Cruz

On Mon, Mar 14, 2022 at 1:48 PM Glenn Nelson @.***> wrote:

I see many messages like this: 2022-03-14 12:57:35,549 - main - INFO: lag-calc has decreased cccsum from 0.668884 to 0.379192 -

I wanted to know where they came from and perhaps understand what they meant. I am sure that it comes from lag_calc.py, vicinity of line 326.

Glenn Nelson in Santa Cruz

On Mon, Mar 14, 2022 at 1:15 PM Calum Chamberlain < @.***> wrote:

Hi Glenn, thanks I will close this PR and open a new one with that change based on develop.

when you clone, you clone the whole repository (all of the branches), and you should either have checked out develop and made changes there, then made a pull request to develop, OR made changes on master and made a pull request to master. The way EQcorrscan does its branching is to maintain master as a current-version branch, and all changes are made to develop then merged to master before a new release (apart from documentation changes which are made directly to master so that the docs get updated immediately). Apologies, it isn't a great model! Nor is it well described.

Error messages (Logger.error) should never appear as INFO type. Are you sure that line is the source of your INFO messages?

— Reply to this email directly, view it on GitHub https://github.com/eqcorrscan/EQcorrscan/pull/485#issuecomment-1067244342, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACHQDVN6DZIHQ65X2ULMW33U76M6FANCNFSM5QRANW6Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

calum-chamberlain commented 2 years ago

I'm not sure what is going on with that INFO message - the format of the messages should be that the Logger is named after the file that generates to log message. Your logger appears to be called __main__ which I wouldn't have expected.

RE template naming - the regular expression: ^[-a-z_0-9]+$ is checked against, which allows lower-case letters, numbers and _ and - characters. This is done to ensure portability of termplates across operating systems when written into Tribes. Windows doesn't allow :s, and is (generally) case insensitive. You should be able to use the hyphen character. You should however be able to write to whatever filename you want.

dr-glenn commented 2 years ago

About the INFO message ... I setup both a file handler and a console handler. File handler gets all message levels, console handler gets only INFO and above. This is standard stuff, so I can't see that causing the problems, but here's my setup. Maybe I've done something wrong that causes lag_calc messages to be labeled as coming from main ? fileFormatter = logging.Formatter('%(asctime)s - %(name)s - %(levelname)s: %(message)s') filehandler = logging.FileHandler('mydetect{:04d}_{:02d}.log'.format(args.year,args.month), mode='w') filehandler.setFormatter(fileFormatter) filehandler.setLevel(logging.DEBUG)

consolehandler = logging.StreamHandler()
consoleFormatter = logging.Formatter('%(asctime)s %(levelname)s:

%(message)s') consolehandler.setFormatter(consoleFormatter)

logger = logging.getLogger(__name__)
logger.setLevel(logging.DEBUG)
logger.addHandler(filehandler)
logger.addHandler(consolehandler)

But I also wanted to capture most of those print messages from the C code, so I did this:

Output stdout and stderr to logger

sys.stdout = StreamToLogger(logger, logging.INFO)
sys.stderr = StreamToLogger(logger, logging.INFO)

My own logging messages all appear with the proper level, e.g., DEBUG, WARNING, ERROR. But as I've shown you, ERROR messages from lag_calc are labeled as INFO.

I hope that makes sense to you.

Glenn Nelson in Santa Cruz

On Mon, Mar 14, 2022 at 2:30 PM Calum Chamberlain @.***> wrote:

I'm not sure what is going on with that INFO message - the format of the messages should be that the Logger is named after the file that generates to log message. Your logger appears to be called main which I wouldn't have expected.

RE template naming - the regular expression: ^[-a-z0-9]+$ is checked against, which allows lower-case letters, numbers and and - characters. This is done to ensure portability of termplates across operating systems when written into Tribes. Windows doesn't allow :s, and is (generally) case insensitive. You should be able to use the hyphen character. You should however be able to write to whatever filename you want.

— Reply to this email directly, view it on GitHub https://github.com/eqcorrscan/EQcorrscan/pull/485#issuecomment-1067314839, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACHQDVOOKEVJ3EEIXT2IPV3U76VWHANCNFSM5QRANW6Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

calum-chamberlain commented 2 years ago

I wonder if the StreamtoLogger is picking up messages that should and converting them to INFO. Can you try running without capturing sydout and stderr?

Anyway, the messages mean that running lag-calc has resulted in worse correlations than you had without running lag-calc, which generally shouldn't happen. If you are getting lots of these messages then something has probably gone wrong somewhere and we might need to dig in to why. For that I would suggest opening a new issue.

dr-glenn commented 2 years ago

An answer, but still a puzzle. I turned off these:

Output stdout and stderr to logger

sys.stdout = StreamToLogger(logger, logging.INFO)
sys.stderr = StreamToLogger(logger, logging.INFO)

Indeed, the messages no longer appear as INFO messages, and they only appear in the stdout/stderr file named run.log. That is, I run my program this way: python my_detect.py >& run.log& Logging generates both a console and file output in my main program. The Logger output from lag_calc now only appears on the console, but it is not from the consoleHandler that I declared in my program my_detect.py. Of course not! And I never set a formatter for the root logger, so lag_calc only outputs a message with no module and no level information, like this: lag-calc has decreased cccsum from 0.654930 to 0.290600 - Strange, I thought that the default formatter was more informative. I should have started with this:

logging.basicConfig(level=logging.DEBUG, format='%(asctime)s %(name)-12s %(levelname)-8s %(message)s', datefmt='%m-%d %H:%M', )

Colum, thank you so much for all the time you've spent reading my emails and educating me. Now I'll leave you in peace and try to understand more about lag_cal and those messages.

Oh, one more question. The detection plots are very nice, but I wish I could dynamically recreate them so that I could zoom in. Would also like to make the template overlay partially transparent. I know how to do all this on my own, but are there core or util classes or functions that can do the job?

Glenn Nelson in Santa Cruz

On Tue, Mar 15, 2022 at 3:57 PM Calum Chamberlain @.***> wrote:

I wonder if the StreamtoLogger is picking up messages that should and converting them to INFO. Can you try running without capturing sydout and stderr?

Anyway, the messages mean that running lag-calc has resulted in worse correlations than you had without running lag-calc, which generally shouldn't happen. If you are getting lots of these messages then something has probably gone wrong somewhere and we might need to dig in to why. For that I would suggest opening a new issue.

— Reply to this email directly, view it on GitHub https://github.com/eqcorrscan/EQcorrscan/pull/485#issuecomment-1068543727, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACHQDVOUORH4C3BE7HWV7TLVAEIWPANCNFSM5QRANW6Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

calum-chamberlain commented 2 years ago

Hey Glenn,

All the plotting that is done in EQcorrscan (I think) is handled by functions in eqcorrscan.utils.plotting - you might be able to find what you need there?

C