JOSS Review: Code structure and class hierarchy about dorado HOT 6 CLOSED

Sounds good. I do not think there is an official "pause" mechanism, just let us know once you are happy with the state of the code and I can take another look and then comment on the review PR.

from dorado.

Many thanks again to @gassmoeller for this super helpful and insightful set of comments and suggestions. This too generated a healthy discussion about how we could best incorporate these ideas to make the code more maintainable in the future.

We wanted to share our plan to help clear up these issues and appreciate any further comments or suggestions you may have.

You define particle locations either based on an initial seed location, or based on existing particle locations either from a previous run, or created by the user. But the initial seed location is expected to be handed over during construction of Particle, while existing locations are handed over to run_iteration. This is a bit confusing for a newcomer, but more importantly: It shows that you are not sure who (which function) is responsible for the particle generation. I would suggest to have one function specific for particle generation ...

We agree with this comment and appreciate the insight. Our plan is to define a generate_particles() function that defines the initial locations for the particles within the all_walk_data dictionary. The proposed workflow would be something like:

# initialize the particles class
particles = Particles(params)

# generate particle start locations and define the number of particles to be routed
particles.generate_particles(x_locations, y_locations, num_particles)

# do the particle routing
particles.run_iteration()

In this way the params class will be clearly defined as the class to handle "global" model parameters, the "generate_particles" functions will define the initial location and number of particles to simulate, and the "run_iteration" method will move the particles through the model domain.

A related comment about assigning responsibilities: I do not understand why the run_iteration function is a member of Particle, but the single_iteration function is a member of Tools. single_iteration does the core work of moving the particles, and so I would suspect it to be a member of Particle.

In our discussion about this comment we realized that our naming scheme for the scripts and functions is not well thought out. We plan to clear this up by revising the name of the file particle_tools.py to LagrangianWalker.py as we intend for that file to hold the functions related to the Lagrangian random-walk process that moves the particles. We also are going to rename single_iteration to particle_stepper to make it more clear that the job of this function is to step the particles in space. Upon reflect we considered it confusing to have "run_iteration" and "single_iteration" as functions in the codebase as they sound very similar but do different things. "run_iteration" will be kept as this is used to move a group of particles in the model domain, whereas the "single_iteration"/"particle_stepper" is part of the random-walk framework and is how individual particles step from one cell to another.

On the other hand the functions coord2ind, ind2coord, and unstruc2grid are typical tools functions and I would expect them in the Tools class

With the renaming of particle_tools.py to LagrangianWalker.py we hope that our decision to leave these functions in particle_track.py makes more sense. Our intention is for the particle_track.py to hold functions associated with the model domain (such as the model parameters and these coordinate transforms), while the LagrangianWalker.py file will hold the core functions associated with the random walk.

Currently Tools defines a number of utility functions for the Particle class, and then Particle is derived from Tools. But this is not a typical is-a parent-child relation.

This comment was particularly insightful and really helped us think about the structure of the code and how it can be best organized to be as clear as possible. With this in mind, we plan to remove the Tools class, and instead re-define the methods there as functions. This removes the inheritance structure that was not really proper inheritance in the standard object oriented programming sense. This change will prompt changes to the documentation that should help address the issues raised about the clarity of the documentation and relationship between "Particle" and "Tools".

So thanks again @gassmoeller, we hope that this plan of action sounds reasonable and will address the concerns you brought up in your review. It was really helpful to us and these changes will definitely enhance the ability of the code to be extended in the future. Please alert us if any of these proposed changes are unreasonable or insufficient.

from dorado.

Yes, all of your suggestions make sense to me. Thanks for addressing my concerns. It sounds like you are already starting the rework, do you want to pause the JOSS publication until the main part of the restructuring is done? That would have the benefit that users will not run into incompatible changes once you release the new version.

With the renaming of particle_tools.py to LagrangianWalker.py we hope that our decision to leave these functions in particle_track.py makes more sense. Our intention is for the particle_track.py to hold functions associated with the model domain (such as the model parameters and these coordinate transforms), while the LagrangianWalker.py file will hold the core functions associated with the random walk.

Yes this makes sense. At some point you probably want to have a specific file exclusively for the Particles class (because that is likely to grow), but it makes sense now that coord2ind is not in lagrangian_walker.py.

from dorado.

Yes, all of your suggestions make sense to me. Thanks for addressing my concerns. It sounds like you are already starting the rework, do you want to pause the JOSS publication until the main part of the restructuring is done? That would have the benefit that users will not run into incompatible changes once you release the new version.

Yes, we plan to restructure the code prior to the JOSS publication - am not sure if this requires pausing the submission or not as we believe we can get this done relatively fast (within 1-2 weeks).

from dorado.

That sounds great, thank you!

from dorado.

Hi @gassmoeller, we have incorporated these suggestions and hopefully made all of the associated changes in the documentation, doc-strings and so forth. PR #15, covers the bulk of the changes associated with the review comments. Thanks again for making the suggestions you made, they really helped us improve this project, and for me these comments have helped me think about code structure and object hierarchies in a new light.

I think we are ready for this to be "unpaused".

from dorado.