Closed stevebrasier closed 9 years ago
Thank you for the submission! The review process will start on the 20th.
Hi Steve, the paper build fine for me too, I am glad you could make it work!
Good - many thanks for you help with that, it was a bit of a learning curve!
Steve Brasier Senior Engineer, Energy ATKINS The official engineering design services provider for the London 2012 Olympic and Paralympic Games The Hub, 500 Park Avenue, Aztec West, Almondsbury, Bristol BS32 4RZ | Tel: 01454 66 2737 | Fax: 01454 66 3333 Email: stevebrasier@atkinsglobal.com | Web: www.atkinsglobal.com
From: Pierre de Buyl [notifications@github.com] Sent: 20 August 2014 15:04 To: euroscipy/euroscipy_proceedings Cc: Brasier, Steve Subject: Re: [euroscipy_proceedings] Paper: A Python-based Post-processing Toolset For Seismic Analyses (S. Brasier, F. Pollard) (#33)
Hi Steve, the paper build fine for me too, I am glad you could make it work!
— Reply to this email directly or view it on GitHubhttps://github.com/euroscipy/euroscipy_proceedings/pull/33#issuecomment-52782288.
The IS team in Atkins has scanned this email and any attachments for viruses and other threats; however no technology can be guaranteed to detect all threats. Always exercise caution before acting on the content of an email and before opening attachments or following links contained within the email.
This email and any attached files are confidential and copyright protected. If you are not the addressee, any dissemination of this communication is strictly prohibited. Unless otherwise expressly agreed in writing, nothing stated in this communication shall be legally binding.
The ultimate parent company of the Atkins Group is WS Atkins plc. Registered in England No. 1885586. Registered Office Woodcote Grove, Ashley Road, Epsom, Surrey KT18 5BW. A list of wholly owned Atkins Group companies registered in the United Kingdom and locations around the world can be found at http://www.atkinsglobal.com/site-services/group-company-registration-details
Consider the environment. Please don't print this e-mail unless you really need to.
@pdebuyl - I will review this paper.
I'll review it too!
A Python-based post-processing toolset for seismic analyses By Steve Brasier and Fred Pollard
This paper describes a tool written in Python to make specific easy-to-use functions. The tool uses numpy and matplotlib, in many cases wrapping functions to be more user friendly. The code is much easier to improve and update than the previous version that was not written in Python.
Overall, this paper is nicely written and clearly explains what was wanted in a new tool, why Python was chosen, and how everything worked out. It will be helpful for others who want to do a similar sort of thing for their group. It should be accepted with minor revisions.
Some specific comments/changes for improvement:
inspect
module sounds pretty awesome.Paper: A Python-based Post-processing Toolset For Seismic Analyses (S. Brasier, F. Pollard)
Please rate the paper using the following criteria (please use the abbreviation to the right of the description)::
below doesn't meet standards for academic publication meets meets or exceeds the standards for academic publication n/a not applicable
Some of the graphics could be of better publication format (such as PS/PDF for example) but generally this is a moot point as they are of sufficient resolution. The screenshot is necessarily in a raster form (PNG) so it makes sense that the remaining figures are in the same format.
For the following questions, please respond with 'yes' or 'no'. If you answer 'no', please provide a brief, one- to two-sentence explanation.
No – the product does not appear to be in the open domain
Yes
Yes
Yes – though it would be interesting to know if there are other countries with similar nuclear post-processing requirments.
Yes
Yes
Yes
Yes
Yes
Yes
Yes
Yes
Yes
Not necessarily.
Yes. See inline comments for specific change recommendations.
Dear @kthyng and @pelson , thank you for your reviews.
Dear @stevebrasier , please propose modifications addressing the comments made in the reviews.
Your updates should arrive for mid-october so that the reviewers can assess the update before the end of october.
Thanks I will do. What’s the procedure here?
Make changes
Submit a pull request
Respond to comments
Or 3/ first?
Thanks
Steve Brasier Senior Engineer, Energy
ATKINS 75 years of design, engineering and project management excellence
Tel: +44 (0)1454 66 2737 The Hub, 500 Park Avenue, Bristol BS32 4RZ Email: steve.brasier@atkinsglobal.commailto:firstname.surname@atkinsglobal.com | Web: www.atkinsglobal.comhttp://www.atkinsglobal.com/
From: Pierre de Buyl [mailto:notifications@github.com] Sent: 26 September 2014 09:48 To: euroscipy/euroscipy_proceedings Cc: Brasier, Steve Subject: Re: [euroscipy_proceedings] Paper: A Python-based Post-processing Toolset For Seismic Analyses (S. Brasier, F. Pollard) (#33)
Dear @kthynghttps://github.com/kthyng and @pelsonhttps://github.com/pelson , thank you for your reviews.
Dear @stevebrasierhttps://github.com/stevebrasier , please propose modifications addressing the comments made in the reviews.
Your updates should arrive for mid-october so that the reviewers can assess the update before the end of october.
— Reply to this email directly or view it on GitHubhttps://github.com/euroscipy/euroscipy_proceedings/pull/33#issuecomment-56935235.
The IS team in Atkins has scanned this email and any attachments for viruses and other threats; however no technology can be guaranteed to detect all threats. Always exercise caution before acting on the content of an email and before opening attachments or following links contained within the email.
This email and any attached files are confidential and copyright protected. If you are not the addressee, any dissemination of this communication is strictly prohibited. Unless otherwise expressly agreed in writing, nothing stated in this communication shall be legally binding.
The ultimate parent company of the Atkins Group is WS Atkins plc. Registered in England No. 1885586. Registered Office Woodcote Grove, Ashley Road, Epsom, Surrey KT18 5BW. A list of wholly owned Atkins Group companies registered in the United Kingdom and locations around the world can be found at http://www.atkinsglobal.com/site-services/group-company-registration-details
Consider the environment. Please don't print this e-mail unless you really need to.
Hi Steve,
Any change that you upload to github will automatically update the content of the pull request. So, there is no new pull request to make, everything will appear here. The procedure is thus:
These actions can be done in no particular order. I remain available if further details are needed.
Pierre
For the record, my review was against ac6cbe62c5dda3e42ddf8d9a290c09ff90e6949f and I will be able to diff the changes easily if you push to this PR.
Dear @stevebrasier , it is almost a month since the request for update. May you proceed?
Regards, Pierre
Hi, thank you for the review. Can I ask for a few clarifications before I address the comments? I have added text below – hopefully this is clear.
Regards
Steve Brasier
From: Kristen Thyng [mailto:notifications@github.com] Sent: 17 September 2014 23:11 To: euroscipy/euroscipy_proceedings Cc: Brasier, Steve Subject: Re: [euroscipy_proceedings] Paper: A Python-based Post-processing Toolset For Seismic Analyses (S. Brasier, F. Pollard) (#33)
A Python-based post-processing toolset for seismic analyses By Steve Brasier and Fred Pollard
This paper describes a tool written in Python to make specific easy-to-use functions. The tool uses numpy and matplotlib, in many cases wrapping functions to be more user friendly. The code is much easier to improve and update than the previous version that was not written in Python.
Overall, this paper is nicely written and clearly explains what was wanted in a new tool, why Python was chosen, and how everything worked out. It will be helpful for others who want to do a similar sort of thing for their group. It should be accepted with minor revisions.
Some specific comments/changes for improvement:
"By itself this raw data..." - By data do you mean model output? I prefer to keep these sources of information distinct because they have different connotations.
By “raw data” I mean the binary data on positions, velocities etc provided from LS-DYNA, so yes model output in once sense at least. I’m not sure what “sources” you would like me to keep distinct – can you explain please?
Define VBA the first time it is used
Will fix.
Fig. 1: I prefer to use diverging colormaps when there is a critical value in your plotted field that you are showing a comparison to. For example, this might be 0 for the critical value, which would be set to zero, and deviations from 0 would be plotted in red or blue depending on whether they are positive or negative. So, while I love that you found a better colormap for your use and set it as your default colormap, you might consider a sequential colormap in this case.
Yes – the example in the paper might not be so good actually here. In many cases we do have a critical value, hence the use of a default. To be honest setting up the examples with suitable non-sensitive data took quite a while and I do want to show this colorbar in the paper, so I would prefer to leave this as it is – is this acceptable?
Fig. 1: It looks like you are using the "extend" option on the colorbar, which implies that there are larger and smaller values than what is displayed on the colorbar. If this isn't the case, can the arrows be removed? Or maybe there is another reason to have them there?
This is a good idea. At present the extend option is always turned on, but we should improve the plotter so that it only adds it if necessary. I am not proposing to make this change for the paper though.
Figs. 2 and 3: Can the line widths and/or styles be easily adjusted? That could be convenient for switching between publication-appropriate plots and presentation-appropriate plots, when visibility is very different, and also in case the plot needs to be printed to grayscale.
Theoretically yes, but the full functionality is not in there / exposed to the user at the moment. Is there a change to the paper required here?
Your inspect module sounds pretty awesome.
This is in the stdlib, not something we’ve created – do you mean I need to make this clear?
There is a hanging comma in Section 7
Will fix.
"Such as script uses a ..." This sentence is confusing. Typos?
Will fix
Last sentence seems to have a typo.
Will fix
Hi, sorry for the delay. I have just replied to @kthyng's comments by email for a few clarifications, assuming that will appear here. I've been through @pelson's comments and will address them all. Hoping to get this back next week.
Hi Steve,
I'll clarify in the same order as above:
Thanks! Kristen
Hi @stevebrasier , is your paper ready for an updated review from @kthyng and @pelson ?
Regards, Pierre
Hi, yes I added commits on Wednesday to address both sets of comments (6aba01a and c30a3a2).
regards Steve
@kthyng and @pelson may you comment on the improvements made by Steve in response to your reviews?
Thanks, Pierre
:+1: from me. Nice work @stevebrasier.
Thanks @stevebrasier. Looks good.
On Mon, Nov 17, 2014 at 3:36 AM, Phil Elson notifications@github.com wrote:
[image: :+1:] from me. Nice work @stevebrasier https://github.com/stevebrasier.
— Reply to this email directly or view it on GitHub https://github.com/euroscipy/euroscipy_proceedings/pull/33#issuecomment-63279507 .
Kristen M. Thyng Postdoctoral Research Associate Department of Oceanography Texas A&M University http://kristenthyng.com
Hi all, thanks for the updates. There is no further action needed. The merge will take place after a few routine checks and global checks on the whole proceedings.
Regards, Pierre
Dear @stevebrasier The text in the figures is really small. Can you regenerate the figure with larger fonts for the tick and axes labels and for the legend? Fig. 2, as a screenshot, does not need any change indeed.
Also, if possible, pdf figures are significantly preferred.
Regards,
Pierre
The text size is set up for plots being page width rather than column-width. Unfortunately changing this would require me to modify some source and rerun the post-processing - and I can't do that till January as I don't have access to the appropriate system until then. If mpl can produce pdfs natively (I haven't checked) then I can output pdfs at that point - if it needs additional packages installed that could take several more weeks as it'll have to go via our IT probably. Let me know what the timescale / quality trade-off is!
Is there any reason you can't just edit the files in a program like Illustrator, Pixelmator, etc, etc? I often resize or overlay larger fonts afterward if needed.
On Wed, Dec 17, 2014 at 3:21 AM, stevebrasier notifications@github.com wrote:
The text size is set up for plots being page width rather than column-width. Unfortunately changing this would require me to modify some source and rerun the post-processing - and I can't do that till January as I don't have access to the appropriate system until then. If mpl can produce pdfs natively (I haven't checked) then I can output pdfs at that point - if it needs additional packages installed that could take several more weeks as it'll have to go via our IT probably. Let me know what the timescale / quality trade-off is!
— Reply to this email directly or view it on GitHub https://github.com/euroscipy/euroscipy_proceedings/pull/33#issuecomment-67296153 .
Kristen M. Thyng Postdoctoral Research Associate Department of Oceanography Texas A&M University http://kristenthyng.com
Else, increasing the scales of the figures produces an acceptable print quality. You may check it at http://pdebuyl.be/tmp/steve_brasier_tmp.pdf
To me, this is fine at this point and I will merge unless explicit opposition. For info, anyway, matplotlib indeed can generate pdfs :-)
To Katy: The figures are in png and thus cannot be edited.
Pierre: Sure, you can't do vector edits to the image if it is a png, but you can always edit images by just overlaying new axes with text boxes, for example. This sort of thing is handy for when you can't rerun your images for a month.
But, I don't care whether this happens or not in this paper.
On Wed, Dec 17, 2014 at 2:56 PM, Pierre de Buyl notifications@github.com wrote:
Else, increasing the scales of the figures produces an acceptable print quality. You may check it at http://pdebuyl.be/tmp/steve_brasier_tmp.pdf
To me, this is fine at this point and I will merge unless explicit opposition. For info, anyway, matplotlib indeed can generate pdfs :-)
To Katy: The figures are in png and thus cannot be edited.
— Reply to this email directly or view it on GitHub https://github.com/euroscipy/euroscipy_proceedings/pull/33#issuecomment-67392610 .
Kristen M. Thyng Postdoctoral Research Associate Department of Oceanography Texas A&M University http://kristenthyng.com
Thank you – that is more how they were intended to be displayed so is appropriate in some ways if the paper format can cope with the larger size.
Regards
Steve
From: Pierre de Buyl [mailto:notifications@github.com] Sent: 17 December 2014 20:57 To: euroscipy/euroscipy_proceedings Cc: Brasier, Steve Subject: Re: [euroscipy_proceedings] Paper: A Python-based Post-processing Toolset For Seismic Analyses (S. Brasier, F. Pollard) (#33)
Else, increasing the scales of the figures produces an acceptable print quality. You may check it at http://pdebuyl.be/tmp/steve_brasier_tmp.pdf
To me, this is fine at this point and I will merge unless explicit opposition. For info, anyway, matplotlib indeed can generate pdfs :-)
To Katy: The figures are in png and thus cannot be edited.
— Reply to this email directly or view it on GitHubhttps://github.com/euroscipy/euroscipy_proceedings/pull/33#issuecomment-67392610.
The IS team in Atkins has scanned this email and any attachments for viruses and other threats; however no technology can be guaranteed to detect all threats. Always exercise caution before acting on the content of an email and before opening attachments or following links contained within the email.
This email and any attached files are confidential and copyright protected. If you are not the addressee, any dissemination of this communication is strictly prohibited. Unless otherwise expressly agreed in writing, nothing stated in this communication shall be legally binding.
The ultimate parent company of the Atkins Group is WS Atkins plc. Registered in England No. 1885586. Registered Office Woodcote Grove, Ashley Road, Epsom, Surrey KT18 5BW. A list of wholly owned Atkins Group companies registered in the United Kingdom and locations around the world can be found at http://www.atkinsglobal.com/site-services/group-company-registration-details
Consider the environment. Please don't print this e-mail unless you really need to.
Hi, paper builds OK on my PC so I hope this is ready for submission.