search

passaH2O / dorado

For shallow-water Lagrangian particle routing.
https://passah2o.github.io/dorado
MIT License
54 stars 9 forks source link

JOSS Review: "Particle" class name #12

Closed elbeejay closed 4 years ago

elbeejay commented 4 years ago

This was a comment made by @gassmoeller here:

Your main class that handles all particles is called "Particle". When I looked into the code I expected this to be a class for one single particle. In terms of nomenclature, maybe something like ParticleHandler, or ParticleManager seems more appropriate (I do not insist on those names, I only suggested them, because in my own particle implementation I chose the name ParticleHandler). At least I would consider using "Particles" to make clear that it is a class that holds more than one particle. I realize this may be a disruptive change, and I do not know how much backward compatibility you have to consider, but I would like to hear your opinion on the topic.

elbeejay commented 4 years ago

We appreciate the insightful review and this comment prompted a productive discussion about the overall naming scheme for several of the classes and functions in the dorado code base.

Our plan is to change Particle to Particles to acknowledge that this class holds a set of particles not just an individual one. In addition, we plan to rename single_iteration to particle_stepper to be more descriptive and indicate that the purpose of the function is to move the particles around. We are intend to rename params to modelParams to indicate that the parameters defined in that class are related to the "global" model and not a particular single particle or set of particles.

Similarly, we plan to rename particle_tools.py to LagrangianWalker.py to indicate that the purpose of the file is to hold the functions related to the Lagrangian transport of the particles.

@gassmoeller - do you think these changes would improve the clarify of the classes and functions? Thank you again for the very helpful suggestion!

gassmoeller commented 4 years ago

Yes the suggested names sound very reasonable. You may want to change LagrangianWalker.py into lagrangian_walker.py to stay consistent with your naming and capitalization scheme, unless you had a specifc reason to make it CamelCase.

elbeejay commented 4 years ago

We have gone through and made the proposed changes. As suggested above, we named our random walk script lagrangian_walker.py. The bulk of changes associated with this issue are in #15.

gassmoeller commented 4 years ago

Looks good, this issue can be closed.