4
\$\begingroup\$

The point of my code is to:

  • Read in the observed data from the a catalog of stars (whitespace separated table).

  • Do some unit conversions on the observed data (math stuff).

  • Apply an interstellar correction factor to the observed data (more math).

  • Compare the scaled observed data to the DUSTY model data using the BC table, using a least-squares approach (comparison of observed data to theoretical models found in a large table).

  • Find the best fit models for the observed data (should be obvious).

I'm trying to rewrite 'spaghetti code' I was handed, that doesn't function quickly, nor does it allow for easy updating (adding different statistical calculations, etc.). I'm pretty new to Python, so I'm struggling with the idea of classes and this is my attempt at it.

class Stellar_setup:
    Fbol=5.01333e-10
    c=2.99792458e+8
    L=1000
    star_list=[]
    dusty_models=np.array([]) #Array of all the dusty models
    incoming_stars='' #List of incoming stars


    def __init__(self):
        """Initiates the below modules."""
        self.star_catalog()
        self.InputKey()

    def star_catalog(self):
        """Imports the star catalog"""
        try:
            star_catalog=raw_input('Input pathname of stellar catalog: ')
            star_list=[]
            with open(star_catalog) as incoming_stars:
                for line in incoming_stars.readlines():
                    x=[item for item in line.split()]
                    star_list.append(x) #Appends the individual star-IDs to the empty array star_list.
            print 'Stars imported successfully.'
        except IOError:
            print 'Star import unsuccessful. Check that the star catalog file-path is correct. Program exiting now.'
            sys.exit()

    def InputKey(self):
        """Imports the Input (.inp) file. Historically, InputUt.inp.
        Allows for definition of parameters which are utilized later in the script."""
        input_script=raw_input('Pathname of .inp file: ')
        InputKey=[]
        try:
            with open(input_script) as input_key:
                for line in input_key.readlines():
                    x=[item for item in line.split()]
                    InputKey.append(x)
            if InputKey[0][0]=='1' or InputKey[0][0]=='2':  #Checks to see if .inp file is valid or not by checking the first row/column which should be 1 or 2.
                print 'Your .inp file was successfully imported'
            else:
                print 'Your .inp file import failed. Check the validity of your file.\n Program exiting now.'
                sys.exit()
        except IOError:
            print 'The .inp file import was unsuccessful. Check that your file-path is valid.\n Program exiting now.'
            sys.exit()

    # You probably need to put a module here that pulls in the DUSTY tables.







if __name__=='__main__':
    Stellar_setup()

I was thinking about making about three other classes that:

  • Identify necessary attributes from the tables (pulling data from columns/rows)
  • Do analysis of information, and model/data comparison to find the best theoretical models that match up with the observed data. This uses a least-squares approach.

From what I understand classes can 'inherit' attributes from other classes. The way I think I understand this is that my class Stellar_setup will be inherited by my class Attribute_Identification (which haven't been written yet), and so on. I'm not sure if I'm going about this the right way, can anyone clear things up for me.

\$\endgroup\$
  • \$\begingroup\$ Ironically, I read the last sentence first. \$\endgroup\$ – Lstor Jul 21 '13 at 18:21
  • \$\begingroup\$ Damn. Shoulda placed that in the middle. \$\endgroup\$ – Matt Jul 21 '13 at 18:22
  • \$\begingroup\$ The title of your post should be the function/purpose of your code. \$\endgroup\$ – SirPython Mar 22 '15 at 15:00
5
\$\begingroup\$

This looks like procedural code, poorly dressed up in object-oriented style. Python doesn't require you to write object-oriented code, but if you do, you should model it properly.

Think of objects as "smart" data structures. If your classes are well designed, you can ask the objects to perform operations on themselves in such a way that you don't have to worry about the details of how it performs the task.

For example, StarCatalog should definitely be a class of its own. There should probably be a Star class as well. The outline of those classes might look like:

class Star:
    def __init__(self, id, magnitude, right_ascension, declination):
        ...
    def id(self):
        ...
    def magnitude(self):
        ...
    def coordinates(self):
        ...

class StarCatalog:
    def __init__(self):
        ...
    def add_star(self, star):
        ...
    def remove_star(self, id):
        ...
    def get_star(self, id):
        ...
    def __iter__(self):
        ...

class StarCatalogImporter:
    def __init__(self, catalog):
        ...
    def import_file(filename):
        ...

Once your model is defined, you can start using it!

catalog = StarCatalog()

importer = StarCatalogImporter(catalog)
try:
    importer.import_file(raw_input('Input pathname of stellar catalog: '))
    print 'Stars imported successfully.'
except IOError:
    print 'Star import unsuccessful. Check that the star catalog file-path is correct. Program exiting now.'
    raise

corrector = InterstellarCorrector(correction_factor=0.2342)
corrected_catalog = corrector.correct(catalog)

result = least_sq_correlator.correlate(corrected_catalog, dusty_model)

Notice how the classes are designed to separate concerns. The StarCatalogImporter class knows how to add the stars listed in a file into a StarCatalog; once that is done, you can ask the catalog to enumerate its stars. The StarCatalogImporter interface should be as reusable as possible. For example, if it encounters an error while importing, it raises an exception, rather than printing a message and exiting. The code that uses the StarCatalog decides how to handle those situations. (It might print an error message and exit. It might be a GUI application, in which case it would display the error in a dialog box. It might translate the error message into another language. It might try to load a different catalog instead.) On the other hand, the code that uses a StarCatalog should make no assumptions about how the star catalog is implemented. It doesn't know if the StarCatalog internally uses an array, a dict, or a numpy matrix. Therefore, it should only use the functions exposed by the class.

The class interfaces that you design are what prevent your code from being spaghetti. If you design your class interfaces well, you will have a loose coupling, such that you will be able to reuse the class in other programs, and modify the inner workings of the class without disturbing the code that uses your class.

A good rule to follow is that each class should represent one thing. If you can't think of a short, descriptive name for the class, that's a sign that you're on the wrong track. Also, the class name must be a noun; if it's a verb, you're using procedural rather than object-oriented thinking.

\$\endgroup\$
  • \$\begingroup\$ Wow, you're clearing things up for me more than I thought was possible. So let me ask another question. Let's say I pull the data in using a class called 'StarCatalog', the only thing that would do is pull the data in. Then the next class would enumerate it. How exactly (using my code as an example), do you 'put' the data pulled by StarCatalog into the next class, call it 'EnumerateStarData'. And a BIG thank you for such a clarifying write-up. \$\endgroup\$ – Matt Aug 10 '13 at 17:47
  • \$\begingroup\$ In other words, how could corrector.correct(catalog) possibly return another StarCatalog? Good point. My original model, which had StarCatalog emerging from its constructor fully formed, was too simplistic. Catalogs need to be mutable, so that other objects (importers, correctors, or transformations) can manipulate them. I've amended my answer accordingly. (A design using immutable star catalogs would be awkward, but possible, using many subclasses of StarCatalog.) Also, class names must be nouns. EnumerateStarData as a class name indicates that you are still thinking procedurally. \$\endgroup\$ – 200_success Aug 10 '13 at 23:40
1
\$\begingroup\$

I am not used to classes till now either but I can offer some other optimizations for your code.

Firstly you should go over PEP8. These are style conventions but they would help you write code that is considered good. Maybe PEP257 also for how to write docstrings.

You can use list comprehensions - PEP202. They are more efficient and readable. So this

star_list=[]
with open(star_catalog) as incoming_stars:
    for line in incoming_stars.readlines():
        x=[item for item in line.split()]
        star_list.append(x)

can be made this

with open(star_catalog) as incoming_stars:
    star_list = [[item for item in line.split()]
                 for line in incoming_stars.readlines()]

You should read Python tutorials on exceptions specifically this section. In this the part about use of else is relevant here. Combining that with list comprehensions this

InputKey=[]
try:
    with open(input_script) as input_key:
        for line in input_key.readlines():
            x=[item for item in line.split()]
            InputKey.append(x)
    if InputKey[0][0]=='1' or InputKey[0][0]=='2':  #Checks to see if .inp file is valid or not by checking the first row/column which should be 1 or 2.
        print 'Your .inp file was successfully imported'
    else:
        print 'Your .inp file import failed. Check the validity of your file.\n Program exiting now.'
        sys.exit()
except IOError:
    print 'The .inp file import was unsuccessful. Check that your file-path is valid.\n Program exiting now.'                                
    sys.exit()

becomes this

try:
    with open(input_script) as input_key:
        InputKey = [[item for item in line.split()]
                    for line in input_key.readlines()]
except IOError:
    print '''The .inp file import was unsuccessful.
Check that your file-path is valid.\n Program exiting now.'''
    sys.exit()
else:
    if InputKey[0][0]=='1' or InputKey[0][0]=='2':
        print 'Your .inp file was successfully imported'
    else:
        print ''''Your .inp file import failed. Check the
validity of your file.\n Program exiting now.'
        sys.exit()

Note that I used triple quotes to wrap around the stings to be printed. Read PEP 8 for why so be sure to read it. Any text editor with syntax highlighting would offset in reading due to strings being wraped around.

Hope this was helpful.

\$\endgroup\$
  • 2
    \$\begingroup\$ Misses the big picture, and also doesn't answer the question. \$\endgroup\$ – 200_success Aug 10 '13 at 9:51
  • \$\begingroup\$ You might also want to note the pointlessness of [item for item in line.split()]. Except for a side effect that isn't being used, it's equivalent to list(line.split()), which, too, is pointless, because split returns a list anyway, further simplifying it to line.split(). \$\endgroup\$ – icktoofay Aug 10 '13 at 23:31

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.