search

eqcorrscan / EQcorrscan

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

Add path to save plot and some improvement in draw a figure #329

Closed ikahbasi closed 5 years ago

ikahbasi commented 5 years ago

What does this PR do?

add feature to save figure of plot in a directory improve shape of figure

Why was it initiated? Any relevant Issues?

PR Checklist

ikahbasi commented 5 years ago

morning dear Chamberlian. Thanks of you. I test these changes with my own data that i work on it, and it works good! Then i thought it's good for pull request :( sorry. Can i have some automatic test in my Github before make pull request? (like yours) are these free? i like to use kwargs, but i think it need some good and detailed description, because It may confuse users and they have to open source code and search for kwargs. but is it possible to give **kwargs from function to function straightly? and make a good documentation for it.

calum-chamberlain commented 5 years ago

See the section in the online docs about running tests. They are included with the source.

For this, you are documenting the plotdir argument, so that should suffice.

In the docs for the plotting module the kwargs for all plots are briefly described. This should be improved and could be linked to from all the plotting functions.

It is possible to pass kwargs between functions directly, yes.

CJ Chamberlain, out of office

From: iman kahbasi notifications@github.com Sent: Wednesday, September 4, 2019 8:09:22 AM To: eqcorrscan/EQcorrscan EQcorrscan@noreply.github.com Cc: Calum Chamberlain calum.chamberlain@vuw.ac.nz; Comment comment@noreply.github.com Subject: Re: [eqcorrscan/EQcorrscan] Add path to save plot and some improvement in draw a figure (#329)

morning dear Chamberlian. Thanks of you. I test these changes with my own data that i work on it, and it works good! Then i thought it's good for pull request :( sorry. Can i have some automatic test in my Github before make pull request? (like yours) are these free? i like to use kwargs, but i think it need some good and detailed description, because It may confuse users and they have to open source code and search for kwargs. but is it possible to give **kwargs from function to function straightly? and make a good documentation for it.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Feqcorrscan%2FEQcorrscan%2Fpull%2F329%3Femail_source%3Dnotifications%26email_token%3DACTIM45VZ2ANHBL6ALJQSKTQH277FA5CNFSM4ITH3M3KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5ZNIAA%23issuecomment-527619072&data=02%7C01%7Ccalum.chamberlain%40vuw.ac.nz%7Cfa3eaa80ef494b07159308d730aa9e11%7Ccfe63e236951427e8683bb84dcf1d20c%7C0%7C0%7C637031381708994000&sdata=cdfWf2YJ24F3khZHr2NvYnMucr4%2BRIDCxuitncGgVw0%3D&reserved=0, or mute the threadhttps://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FACTIM43OZGRD7TBO62E4WGTQH277FANCNFSM4ITH3M3A&data=02%7C01%7Ccalum.chamberlain%40vuw.ac.nz%7Cfa3eaa80ef494b07159308d730aa9e11%7Ccfe63e236951427e8683bb84dcf1d20c%7C0%7C0%7C637031381709003993&sdata=7M9gThcG1Gu%2BkbSlZbnl2r7vml0YGdaNoVEubj7R4ac%3D&reserved=0.

calum-chamberlain commented 5 years ago

I've updated the plotting function docstrings so that all the supported kwargs are documented in PR #327, I will hopefully merge that into develop today and you should see the change.

The upshot is that size is a supported kwarg for all plotting functions now and is handled by _finalise_figure, so you shouldn't need to add that into the logic here. I think you should just be able to get away with changes to template_gen functions.

calum-chamberlain commented 5 years ago

I updated your fork based on the current develop (with a standardised size kwarg, and docstrings). You will need to patch the template_plot calls as requested for this to work.

By the way, the section of the docs that I meant on testing is here.

calum-chamberlain commented 5 years ago

I'm hoping to get a new release out by the end of this week, it would be great to include this in it - if you can get it done overnight (for me) that would be awesome, otherwise I will just make some patches straight on develop including what you have here. Thanks!

ikahbasi commented 5 years ago

I'm hoping to get a new release out by the end of this week, it would be great to include this in it - if you can get it done overnight (for me) that would be awesome, otherwise I will just make some patches straight on develop including what you have here. Thanks!

morning! I woke up now. and i just saw your message. I try to finish it as soon as i can. But it might take 48 hours, because i don't have the adequate system for testing right now. But i am working on it and editing codes. then i'll commit changes and see your automatic test here today.

calum-chamberlain commented 5 years ago

I;m having a go at this here. I'm really not keen on putting all of this plotdir logic and figure resizing in hard coded into the plotting functions. I wanted that logic to be in the functions that call them (e.g. template_gen), I'm also taking the opportunity to add a consistent plotdir argument to the match-filter objects. I'll make a new PR based on my work in a few minutes (I'm just finishing running the tests) and you can see if that meets your needs.

calum-chamberlain commented 5 years ago

@imankahbasi have a look at #330 and check that that produces the plots that you want.

ikahbasi commented 5 years ago

Hi. How can i find problem of code with continuous-tintegration/appveyor/pr? I want to solve it but i don't know what is problem.

calum-chamberlain commented 5 years ago

It looks like appveyor just got stuck - this sometimes happens - network issues end up causing downloads to hang. However, if #330 achieves the result you want then I would close this and merge that one in favour of this one.

calum-chamberlain commented 5 years ago

Closing in favour of #330