valentineap / pyprop8

A lightweight Python code to calculate the seismic response of a layered half-space (including static components), and derivatives of the wavefield wrt source components.
GNU General Public License v3.0
24 stars 5 forks source link

Consider code reformatting/restructuring #10

Closed valentineap closed 2 years ago

valentineap commented 2 years ago

Comment from @hemmelig in openjournals/joss-reviews#4217:

Briefly – it is clear this is very much a transliteration of the FORTRAN 77 methods, which makes it rather dense reading at times. This is compounded by the fact that the code does not comply with the advised PEP8 style guide. Architecturally, I think it would certainly help to break things out into modules that achieve specific jobs within the package e.g. I would break the layered structure model out from core into a stand-alone module. Having everything within a single _core.py module did make it (personally!) a bit of a task to construct a mental map of the code.

While this doesn’t impact the functionality of the software, I would at the very least recommend running the source code through an automatic formatter such as black. This was the first thing I did when digging into the code myself, and it significantly improved its readability. In terms of refactoring to make use of more pythonic solutions, this wouldn’t really impact the code semantically, so I don’t think it is important enough to invest the time into.

Another critical issue I would like to see/help address is to properly document all functions/methods within the package, not just the high-level interface functions. Not only would it be useful to fully describe all function arguments, there were a number of places where I found it difficult to map what was being calculated to equations set out in the original papers. In particular, the source vector functions, and especially the m2/m6 components of the propagate functions. By and large, I was able to map most things to the equation definitions and confirm everything was as expected, but there were one or two places I wanted to better understand.

Run code through auto-formatter e.g. black to improve readability. I would be happy to submit a pull request doing this. (NON-CRITICAL) Consider revising architecture to make more logically segmented. Document internal functions as well as user-facing ones. Provide specific references at the relevant points in functions to equations in the original papers. It might also be nice to provide the user with some base plotting utilities, but I believe what each function returns to be sufficiently documented that it is not unreasonable to expect a user to handle this themself.

valentineap commented 2 years ago
valentineap commented 2 years ago

I've done various reorganisations of the code base. The main goal was to separate out 'user-facing' functions from the 'backend'. I've also tried to ensure that everything has some basic documentation, highlighting links to equations in the papers. A mathematica notebook (& pdf version) in /notes/ explains where some pieces come from that are expressed in a different form from the paper.

I have thought about providing plotting scripts. However, these would really be very basic (plt.plot(time, seismogram[i,0,:])). Given the goal for pyprop8 to be minimal in scope I think it better not to add anything here. There are a number of scripts/notebooks in the examples directory that demonstrate how to plot things, should a user seek some guidance.

No doubt more could be done to address the various comments here, but I hope that the changes are an improvement and sufficient to help someone make sense of the package.