3
\$\begingroup\$

First, the context:

I'm working with a student to build a web frontend for a star cluster simulation package called starlab. Typical starlab usage involves chaining sequences of commands together with Unix pipes. We want to store both the output and the set of commands (and arguments) used to generate the output in a database.

The specific piece of the puzzle I'm asking about here is representing the commands. Each command has a different set of arguments, some of which take values and others don't. Additionally, some of the commands have arguments for which defaults aren't supplied, so they need to be included.

Since the commands form a predefined, discrete set, it seemed appropriate to use an Enum. Currently, I keep the commands and their arguments separate (stored in the Run object as tuples). Typical usage would look something like:

foo = Run()
foo.set_creation_command(StarlabCreationCommand.makeking, **makeking_args)
foo.set_transform_command(StarlabTransformationCommand.scale, **scale_args)
...
commands = foo.generate_command_list()

My questions:

  1. Does this seem like a reasonable approach to this problem?
  2. Should I roll the argument handling into the StarlabCommand Enum class, rather than storing the arguments in a tuple/dictionary?
  3. Is there anything here that could be improved?

The code:

from enum import Enum

class StarlabCommand(Enum):
    """Enumerator class for starlab commands.

    Each member takes three parameters:

    1. A dictionary of required arguments (with default values),
    2. A list of optional arguments that take a value, and
    3. A list of optional arguments that don't take a value.

    If there are parameters which are not, strictly speaking, required
    (i.e., the underlying starlab command will execute without them being
    supplied) but I want to make sure they get into the database, I will
    include them in the required list. The most common example of this is
    random seed for those commands that use one.
    """
    def __init__(self, required, with_value, without_value):
        """Initialize."""

        self.required = required
        self.with_value = with_value
        self.without_value = without_value

    def build_command(self, **cmd_args):
        """Build a command list suitable for passing to subprocess.Run()"""

        command_list = [self.name]
        for arg, default in self.required.items():
            val = cmd_args.get(arg, default)
            command_list.extend(['-'+arg, val])
        for arg in self.with_value:
            val = cmd_args.get(arg, None)
            if val is not None:
                command_list.extend(['-'+arg, val])
        for arg in self.without_value:
            val = cmd_args.get(arg, False)
            if val:
                command_list.append('-'+arg)
        return command_list

class StarlabCreationCommand(StarlabCommand):
    """Starlab cluster creation commands.

    The required args dictionaries here don't include the number of stars or the
    random seed, which are required of all these commands and are passed in the
    same way in all cases.
    """

    makesphere = ({}, ['R'], list('ilouU'))
    makecube = ({}, ['L'], list('ilou'))
    makeplummer = ({}, ['m', 'r'], list('iRou'))
    makeking = ({'w':5.0}, ['b'], list('iou'))

    def __init__(self, required, with_value, without_value):
        """Initialize.

        All creation methods require a number of stars, and I'm adding
        random seed to the required list.
        """
        super().__init__(required, with_value, without_value)

        self.required['n'] = 500
        self.required['s'] = 123456789

class StarlabTransformationCommand(StarlabCommand):
    """Starlab cluster transformation commands.

    There are more than these, but these are the ones we're supporting for now.
    Unlike the case of creation commands, not all of these require a random seed
    (and in fact, scale uses the s flag for something else) so I can't just put
    that into an __init__() method.
    """
    makemass = ({'e':-2.35, 'f':1, 's':123456789}, list('hlu'), ['i', 'm'])
    makesecondary = ({'s':123456789}, list('flmMu'), list('iIqS'))
    scale = ({}, list('eEmqr'), ['c', 's'])
    makebinary = ({'s':123456789}, list('felou'), [])

class StarlabIntegrationCommand(StarlabCommand):
    """Time integration"""
    kira = ({'d':1, 's':123456789, 't':10},
            list('bDefFgGhIkKlLnNqRTWXyzZ'),
            list('aABEioOrSuUvx'))

class ArchivedRun(Base):
    """Class for archiving Run objects via SQLAlchemy."""
    __tablename__ = "runs"

    run_id = Column(Integer, primary_key=True)
    creation_command_string = Column(String, length=80)
    creation_command = Column(Enum(StarlabCreationCommand))
    creation_command_args = Column(String, length=80)
    random_seed = Column(Integer)
    n_stars = Column(Integer)
    creation_scale_1 = Column(Float)
    creation_scale_2 = Column(Float)

    def __repr__(self):
        return "<Run %d>" % self.run_id

class Run(object):
    """Metadata for a cluster simulation."""

    def __init__(self, random_seed=None, nstars=500):
        """Initialize."""
        self.creation_command = (None, None)
        self.transform_commands = []
        self.integration_command = (None, None)
        if random_seed is None:
            self.random_seed = uuid.uuid4().time_low
        else:
            self.random_seed = random_seed
        self.nstars = nstars

    def set_creation_command(self, creation_command, **args):
        """Set the creation command."""
        self.creation_command = (creation_command, args)

        self.nstars = args.get('n', self.nstars)
        self.random_seed = args.get('s', self.random_seed)

        self.creation_command[0].required['n'] = self.nstars
        self.creation_command[0].required['s'] = self.random_seed

    def add_transform_command(self, transform_command, **args):
        """Add a transformation command.

        There are some special caveats here:

        1. If we're making a cluster that includes binaries, both makesecondary
        and makebinary are required, in that order.
        2. If we're additionally using scale, it should happen between the
        makesecondary and makebinary commands
        """
        if 's' in transform_command.required.keys():
            transform_command.required['s'] = args.get('s', self.random_seed)
        self.transform_commands.append((transform_command, args))

    def set_integration_command(self, integration_command, **args):
        """Set the integration command and its arguments."""
        self.integration_command = (integration_command, args)

    def generate_command_list(self):
        """Build the list of commands for execution by Popen."""
        all_commands = [self.creation_command]
        all_commands.extend(self.transform_commands)
        all_commands.append(self.integration_command)

        command_list = [cmd[0].build_command(cmd[1]) for cmd in all_commands]
        return command_list
\$\endgroup\$
1
\$\begingroup\$

Here is another view on your class structure, in an attempt to make it both more readable and more pythonic so to speak. Let us start of with a yUml diagram of your class structure (somewhat simplified):

Original class diagram

As stated my main issue, is that the actual commands are hidden within the class structure, and I would rather make them separate classes. And I would focus on making the various options clearer. One way to do this is to change into a structure like the following diagram:

New revised class diagram

The changes I've done in this revised diagram are the following:

  • Removed the dependency on Enum and made the different commands into full blown classes within the hierarchy
  • Simplified the names of the classes, just to keep the naming simpler. Since everything would be in the starlab package a full reference would still be something like starlab.MakeBinary(upper_mass_limit=3, ...) or if you moved the different steps into packages of their own, it could become starlab.Transform.Scale(...).
  • Allowed each command to have a set of options related to it, which are instantiated at the node level into the actual commands
  • In the Simulation class you could now also add type safety that you only add intermediate steps of the Transform class, and similar for Creation and Integration. And in these classes you also have method or validators ensuring proper sequencing of transformation steps or similar.
  • Sprinkling __format__() all over the places to allow for code similar to the following:

    a_king = starlab.MakeKing(depth=5, particle_numbers=10000)
    print('Command: {0:cmd}\n\nHtml:\n {0:html}'.format(a_king)
    

    Which would result in something like:

    Command: makeking -d 5 -n 10000
    
    Html:
    <div class="Command Creation">
        <label>MakeKing</label>
        <div class="Option">
            <label>Depth</label>
            <input>5.0</input>
            <input type="button"> Remove option</input>
        </div>
        <div class="Option">
            <label>Particle numbers</label>
            <input>10000</input>
            <input type="button"> Remove option</input>
        </div>
        <div class="add_option">
           <select>
              <option>Depth</option>
              <option>Upper mass limit</option>
              <option>Lower mass limit</option>
              <option>...</option>
            </select>
         </div>
    </div>
    

    The html code is of course way off, and needs to be tailored to your liking. The gist of the idea is that the different class holds options and descriptions which are easily displayed in html code with proper identifiers and links so that you if you add an option it is immediately available on the web page as it automatically knows how to handle that kind of option.

    For more information on __format__ see PEP3101 in the section "Controlling Formatting on a Per-Type Basis", or alternatively PEP 3101: Advanced String Formatting (for Python 2) or (for Python 3).

  • At higher levels than the Option, one would propage the format down to the specific sub objects, and join the different parts apropriately. I.e. The Creation would allow for changing the creation command in the html output, whilst Transform would allow for adding and removal of steps (possibly), and so on.

Example of class MakeKing

The following example is a simplistic implementation of part of the MakeKing class.

class MakeKing(Creation): """Wrapper class for the makeking command, starlab v 4.4.1.

See http://sns.ias.edu/~starlab/tools/auto/makeking
"""

def __init__(self, *args):
    super().__init__()

    options.add("depth", "-d", 5.0, float, 
        "Specify number of particles [no default] " +
        "if unspecified assumes an input snapshot with masses.")
    options.add("particle_number", "-i", bool,
        "Number the particles sequentially [don't number]")
    ...

    for name, value in args.items():
        if name in options:
           options[name].set(name, value)

And this would be used in a Simulation like in my other answer, with the exception of using the basic print statement on the entire simulation to actually get the unix command ready for popen or html text ready for presentation and handling on a web page.

Hope this is somewhat clear, and gives you some good ideas on an alternate approach to your issue at hand.

\$\endgroup\$
2
\$\begingroup\$

The primary reason for using Enums for anything is to give names to constants; this allows easier specification when coding (StarlabIntegrationCommand.kira instead of ...?), and nicer reprs and easier debugging because you get the name instead of a number, or whatever.

Given that each command is represented separately, along with its flags and options, I think you're okay here.

The only constructive criticism I would offer:

  • name the module starlab
  • remove Starlab from the Enum class names
  • remove Command from the Enum class names
  • remove make from enum member names
  • possibly rename Enums from nouns to verbs

With those changes code might look like:

from starlab import Create, Transform, Integrate, Run

foo = Run()
foo.set_creation_command(Create.king, **makeking_args)
foo.set_transform_command(Transform.scale, **scale_args)
foo.set_transform_command(Transform.mass, **mass_args)
...
commands = foo.generate_command_list()

Which should help keep the lines from getting too long (and numbing... and save on the wrists ;) .

\$\endgroup\$
  • \$\begingroup\$ Regarding your suggestion to remove make from enum member names, the member names reflect the names of the underlying tool names in starlab, so I probably won't go that route (though I recognize the urge for brevity). \$\endgroup\$ – tachycline Feb 19 '16 at 1:19
1
\$\begingroup\$

This answer is divided into two parts:

  1. A critical review regarding the chosen class structure
  2. Some general comments to the actual code

Chosen class structure

Let me start by stating, that I might be failing to understand your design goal and aimed audience, but as understand from the scarce information you've given I'd say that to you've chosen to design your solution around the mechanics of the actual commands to be run, and not around the star simulation you want to simulate using starlab.

The entire Run class is kind of the mechanics, and not anything related to the star clustering which should be the main focus. In other words, when you want to work with stars and simulations related to the stars, you have to learn something entirely else like set_creation_command and set_transform_command and so on? Where are the stars and clusters?

Similarily for StarlabCommand and StarlabCreationCommand, you've built a shell for the commands, I presume, but I would much rather build a domain model which could display the options available, and allow for the actual commands to be executed at some point be more hidden in the implementation.

Even the example code you've given at the start, doesn't display what is happening, which I assume is something similar to the first example from the starlab tools pages:

(makeking -w 5 -n 10000
| makemass -l .2 -u 20 -x -2.35
| scale -s
| kira -t 100 -d 1 -D 10 > snapshot ) >& log

I would much rather see something like:

import starlab
simulation = starlab.Simulation()
simulation.intial(starlab.King(depth=5, particle_numbers=10000))
simulation.add(starlab.MakeMass(lower_mass_limit=0.2, upper_mass_limit=20, exponent=-2.35))
simulation.add(starlab.Scale())  # Defaulting to simulate the '-s'
simulation.final(starlab.Kira(timespan=100, log_interval=1, snapshot_interval=10))

simulation.create_snapshot("log.txt") # Or run(...), this executes it

Compare the code above, with your code example with foo.SetCreationCommand(...), and consider which one is easier to maintain as a simulation, and which one is easier to understand related to the actual simulations you'll want to run.

In my mockup, I've initial, add and final to indicate the different parts of the simulation. The starlab.Simulation class would then be in charge of gathering all the commands, which could be done through using (__repr__ or) __str__ to actually build the commands, making the overall logic of the simulation class simpler and it can focus on stuff related to actually running the simulation in the unix shell.

General comments on code

Here are some comments on your coding style:

  • Good naming – You seem to follow most of the guidelines related to naming classes, methods and variables. Given your design domain, most variables are sensible.
  • Good docstrings – You are using docstrings, which are good on its own. Most of these seems to be reasonable and listing some of the vital points related to your chosen class structure.
  • Introduce more vertical spacing – I would use a little more vertical spacing. Specifically I would use 2 newlines before methods to make them stand out a little more, and I would newlines before for, if & co, while, and logical blocks within a method.
  • What are the Starlab* stuff? – For the different parts you've introduced classes listing most likely the default parameters of the different commands as class variables. But you don't list the available parameters, and you don't describe the parameters nor commands. In other words, after reading your Starlab classes I'm left with a question of what is the stuff, and how am I supposed to use it.

And that last point also kinds of sums up this review. You've written code which looks OK, but I don't know how I'm supposed to use it. It kind of seems like I could just as easily write the commands on the command line, and that this interface to the Starlab package is just hiding or making it worse for me to run the simulation.

\$\endgroup\$
  • \$\begingroup\$ Your first comment is correct. This is essentially all plumbing. We've had troubles in the past several years with students getting up to speed using the command line in general and starlab in particular, and so now we're trying to layer a web interface on top of it. As such, these classes won't be used directly by end users, but by the Flask (or Django, or whatever) interface we wrap around it. Likewise, the description of each command and its flags would be handled by the presentation layer of the web app. \$\endgroup\$ – tachycline Feb 19 '16 at 1:14
  • \$\begingroup\$ @tachycline, Even more reason to use a class structure which could easily extend into web components, and later on be used directly to build appropriate command chains... \$\endgroup\$ – holroy Feb 19 '16 at 1:16
  • \$\begingroup\$ You are probably right. I think I was trying to leverage abstraction to avoid some work, but it has to happen somewhere, and this is probably as good a place as any. Thanks for your input. \$\endgroup\$ – tachycline Feb 19 '16 at 19:01
  • \$\begingroup\$ @tachycline After looking on it some more, I see some of the structure I like, and there is possibilities to save work on both the backend and the frontend building good structures. But I would like to get a lot better description of the arguments which are nearly non-present currently. \$\endgroup\$ – holroy Feb 19 '16 at 19:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.