Python class προτάσεις βελτίωσης

[Dr.Fuzzy]

Η παρακάτω κλάση αποτελεί μέρος ερευνητικής εργασίας και ουσιαστικά την έκανα port από κώδικα MATLAB όπου αρχικά είχα γράψει. Την κάνω ποστ εδώ και όποιος έχει όρεξη να την δει με ενδιαφέρει να μου επισημάνει λάθη ή βελτιώσεις αναφορικά με το coding style και ειδικά στο OOP κομμάτι, πχ., κακές πρακτικές κλπ που μπορεί να χρησιμοποιώ. Επισημαίνω ότι functionally είναι ΟΚ.

 

 

#!/usr/bin/python
# -*- coding: utf-8 -*-
"""
Created on Mon Nov  2 23:52:18 2015
 
@author: Dr. Fuzzy
"""
 
import numpy as np
import matplotlib.pyplot as plt
 
class Metrics:
    'Class that contains all metrics'
    mrt = 0
 
    def __init__(self, time, gamma, phi):       
        self.__time = time
        self.__gamma = gamma
        self.__f = phi        
        self.__c = 0
        self.__time = 0
        self.__archive_D = 0
        self.__archive_a = 0
        self.__archive_K = 0
        self.__archive_u = 0
        
    # This method computes the mRT cost 
    def mrt_cost(self, archive_D, archive_a):
        self.__archive_D = archive_D
        self.__archive_a = archive_a
        #(N,o) = self.__archive_D.shape
        #(M,o) = self.__archive_a.shape
        N = len(self.__archive_D)
        M = len(self.__archive_a)
    
        if M == N:
            Metrics.mrt = np.zeros([N, 1])
 
            for i in range(0, N):
                if self.__archive_a[i] == 0:
                    g = 1000
                else:
                    g = self.__archive_D[i] / self.__archive_a[i]
    
                if g < 0.8:
                    mrt_tmp = np.exp(self.__gamma*(g-self.__f))
                else:
                    mrt_tmp = 1+self.__gamma*(g-self.__f)      
 
                Metrics.mrt[i] = mrt_tmp
        
            return Metrics.mrt    
        else:
            print("What is going wrong with you?")
            return
 
    # This method plots the mRT cost
    def plot_mrt(self):
        plt.plot(Metrics.mrt,'-b', linewidth=1.5)
        plt.xlabel('time interval')
        plt.ylabel('mRT (s)')
        #plt.box(on=True)
        plt.show() # force plot now and then return to main code
 
    # This method plots the CPU (%) allocation
    def plot_cpu(self, archive_a, archive_u):
        self.__archive_a = archive_a
        self.__archive_u = archive_u
        plt.plot(self.__archive_a, '-b', linewidth=1.5)
        plt.plot(self.__archive_u,'k', linewidth=1.5)
        plt.xlabel('time interval')
        plt.ylabel('CPU (%)')
        #set(gca,'FontSize',14);                
        #plt.box(on=True)
        plt.show() # force plot now and then return to main code
 
    # This method plots the gain
    def plot_gain(self, archive_K):
        self.__archive_K = archive_K
        plt.xlabel('time interval')
        plt.ylabel('gain')
        plt.plot(self.__archive_K,'-b*')
        #set(gca,'FontSize',12);
        #plt.box(on=True)
        plt.show() # force plot now and then return to main code
 
    def rms_error(self, archive_u, archive_a, c):
        self.__c = c
        self.__archive_u = archive_u
        self.__archive_a = archive_a
        return np.sqrt(np.linalg.norm(self.__archive_u - self.__c * self.__archive_a)**2 / self.__time)
    
# Test
#metrics=Metrics(100, 10, 0.8)
#x = metrics.mrt_cost(np.array([1,2,3,4]).T,np.array([5,6,7,8]).T)
#metrics.plot_mrt()

[groot]

Εντάξει φαίνεται

 

 

Αυτό που θα έκανα, εάν έκανα κάτι, θα ήταν να έχω μία μέθοδο _plot_metric και να περνάω σε αυτήν ορίσματα για το plotting των μετρικών.

 

Όπως είναι τώρα έχεις γράψει πάνω από μία φορά το plt.plot. Δεν θα ήταν καλύτερα να τα περνάς ως όρισμα; Π.χ.

 

def _plot_metric(x_ax, y_ax, values_labels_list, include_legend=False, save_to_file=Null, file_name=Null)

ή:

 

_metrics = {
  'mtr': {'val': [Metrics.mrt], 'xlabel': 'A x label', 'ylabel': 'A y label'},
  'cpu': {'val': [self.__archive_a, self.__archive_u] ....},
  'gain': {....}
}

def _plot_metric(metric_name) {
  val_to_plot = _metrics[metric_name]['val']
  # etc
}

 

Δηλαδή, οργάνωσε κάπου τα κοινά και έχε κάπου κώδικα για να κάνει μία δουλειά. Π.χ., plotting. Δεν είναι ανάγκη να κάνεις plt.plot τόσες φορές.

 

Εάν υιοθετήσεις το dict approach, θα μπορούσες να το περνάς και ως **kwargs κατευθείαν στην function/method.

 

 

Τέλος, εάν χρησιμοποιείς PyCharm, βάζε σχόλια και προσδιόριζε τύπους ορισμάτων και επιστροφής!!!! Βοηθάνε τρελά στο auto-completion.

[mad-proffessor]

Θέλει περισσότερη αφαίρεση π.χ ξεχωριστή κλάσση για plotting άλλη για metrics. Αν έχεις χρόνο δες το mtv pattern της python,

αν όμως υπάρχουν στενα περιθώρια στη παραδοση της εργασίας δε βρίσκω κάτι το κακό, προχώρα.

[Dr.Fuzzy]

@groot 

Καλή ιδέα, μου αρέσει, θα το φτιάξω!

 

@mad-proffessor

Θα το κοιτάξω. Ναι γενικά το σκεφτόμουν αυτό που λες αλλά κάπου θεώρησα ότι δημιουργώ πολύ clutter και oversimplification...

 

Στενά περιθώρια όχι. Γενικά το συγκεκριμένο πρόκειται να γίνει extension υπάρχουσας δουλειάς και επειδή θέλω να κινηθώ προς "open source" προκειμένου να "δέσει" με άλλα open source frameworks, αποφάσισα να το κάνω port από το MATLAB όπου αρχικά το γράψαμε.

[pmav99]

Python 2 ή 3; Μάλλον 3, αλλά αν 2 τότε γιατί old style classes; Το να κληρονομείς από object είναι καλή συνήθεια για να είναι cross version compatible ο κώδικάς σου.

 

To mrt γιατί είναι class attribute; Δεν λέω ότι είναι λάθος αλλά είναι σίγουρα αυτό που ήθελες; Πόσα instances θα δημιουργήσεις;

http://stackoverflow.com/questions/8959097/what-is-the-difference-between-class-and-instance-variables-in-python
 

Οι πιο πολλές μεθόδοι σου κατ' ουσίαν είναι staticmethods...

 

Αντί για comments χρησιμοποίησε docstrings.

 

Οι μεθόδοι είθισται να είναι ρήματα (πχ calc_whatever()).

 

Χρησιμοποίησε descriptive variable names ή/και document αυτά που γράφεις.

 

Γιατί κάνεις assignment στις plot_*();

 

PEP8!

Αλλάζεις type στο mrt (από integer σε numpy array). Εν γένει κακή πρακτική. Αν πρέπει να το αρχικοποίησεις μετέτρεψέ το σε None.

[mad-proffessor]

Clutter θα δημιουργησεις αν αργοτερα θελεις να επεκτεινεις τις δυνατοτητες της κλασσης και βαλεις νεες μεθοδους σε αυτη χωρις να εχεις λογικα τμηματοποιησει τη δομη της οπως σου περιεγραψα. Το MTV της python ειναι ουσιαστικα το πασιγνωστο Mvc pattern.Τελος ο pmav εχει δικαιο σε αυτα που τονιζει αλλα ειναι λιγο αυστηρος(σε λογικα πλαισια μιας κ εσυ το ζητησες).

[k33theod]

H np.zeros όπως και η .ones και η .empty δεν παίρνει κανονικά άγκιστρα αλλά παρενθέσεις στις διαστάσεις του πίνακα

Δηλαδή (2,3) και όχι [2,3]

Όταν κάνεις μονοδιάστατο μπορείς να δώσεις ένα άριθμο δηλαδή np.zeros(10)

Εσύ κάνεις Ν γραμμές μία στήλη. Αν δεν υπάρχει λόγος μπορείς να το αλλάξεις σε np.zeros (N)

[pmav99]

 

H np.zeros όπως και η .ones και η .empty δεν παίρνει κανονικά άγκιστρα αλλά παρενθέσεις στις διαστάσεις του πίνακα

Δηλαδή (2,3) και όχι [2,3]

 

Ποια η διαφορά; Iterables είναι και τα δύο...

[Dr.Fuzzy]

Να είσαστε όσο αυστηροί θέλετε δεν έχω τέτοια θέματα, άλλωστε εγώ το ζήτησα. Ευχαριστώ που ασχολείστε! 

 

@pmv99

• python 2.7 και χρησιμοποιώ το Spyder IDE.
• old style classes...σε έχασα!
• Για το mrt και μένα δε μου φάνηκε εξ'αρχης πολύ κομψός ο τρόπος. Έτσι όπως είναι τώρα, ένα instance, αλλά πιθανών να χρειαστούν 2 αργότερα.
• Για τα variable names, ναι το ξέρω, αλλά τα κράτησα όπως ήταν στον MATLAB κώδικα για consistency. Ίσως να τα αλλάξω.
• "Γιατί κάνεις assignment στις plot_*();"....Δεν το έπιασα; Εννοείς γιατί περνάω ορίσματα;
• Αυτά με τα σχόλια...πω πω κάτι το OCD μου, με σκότωσες...θα τα φτιάξω σίγουρα!
• Ναι static είναι...θα μπορούσα να τις δηλώσω @staticmethod, αλλά μου δημιουργεί κάποιο σοβαρό πρόβλημα αν δεν;

@k33theod

Θα το δω.

 

@mad-proffessor

Αν κάνω την πχ, MetricsPlot, ξεχωριστή κλάση, τότε μήπως θα πρέπει να κάνει inherit την Metrics ή όχι;

Επεξ/σία 23 Μαρτίου 2016 από Dr.Fuzzy

[k33theod]

 

 

Ποια η διαφορά; Iterables είναι και τα δύο...

 

Βασικά δεν ξέρω απλά στην συνάρτηση λέει shape : int or tuple of int

https://docs.scipy.org/doc/numpy-dev/reference/generated/numpy.zeros.html#numpy.zeros

 

Οπς γράψε λάθος αυτό το λέει στο empty στο zeros λέει shape : int or sequence of ints 

Shape of the new array, e.g., (2, 3) or 2. Οπότε κολάει και ο πίνακας

Νόμιζα είναι ίδια

[groot]

Ρε pmav99.... Ωραίος ρε! Δεν έχει και up vote

[pmav99]

@Dr_fuzzy

και για να μη λες ότι κάνω μόνο κριτική, εδώ είναι το πως θα το έγραφα εγώ:

#!/usr/bin/python
# -*- coding: utf-8 -*-
"""
Created on Mon Nov  2 23:52:18 2015

@author: Dr. Fuzzy
"""

from __future__ import division
from __future__ import print_function
from __future__ import absolute_import

import numpy as np
import matplotlib.pyplot as plt


class Metrics(object):
    """ Class that contains all metrics """

    def __init__(self, time, gamma, phi):
        self.time = time
        self.gamma = gamma
        self.phi = phi
        self.mrt = None

    def calc_mrt_cost(self, archive_D, archive_a):
        """ Calculate the mRT cost """
        # sanity check
        if len(archive_D) != len(archive_a):
            raise ValueError("Archives with different length: <%r>, <%r>" % (archive_D, archive_a))
        # local names (if you care for speed, you'd better avoid attribute lookups in inner loops!)
        gamma, phi = self.gamma, self.phi
        self.mrt = mrt = np.zeros_like(archive_D, dtype=np.float)
        # main algorithm
        # XXX I don't like how I named the variables here, but I don't know the topic so
        # I can't come up with better ones!
        for i, (a, D) in enumerate(zip(archive_a, archive_D)):
            g = D / a if a != 0 else 1000
            k = gamma * (g - phi)
            mrt[i] = np.exp(k) if g < 0.8 else 1 + k
        return mrt

    def plot_mrt(self):
        """ Plot the mRT cost. """
        if self.mrt is None:
            msg = "mRT cost has not been calculated. Call <calc_mrt_cost()> and try again!"
            raise ValueError(msg)
        plt.plot(self.mrt, '-b', linewidth=1.5)
        plt.xlabel('time interval')
        plt.ylabel('mRT (s)')
        #plt.box(on=True)
        plt.show() # force plot now and then return to main code

    @staticmethod
    def plot_cpu(archive_a, archive_u):
        """ Plot the CPU (%) allocation. """
        plt.plot(archive_a, '-b', linewidth=1.5)
        plt.plot(archive_u, 'k', linewidth=1.5)
        plt.xlabel('time interval')
        plt.ylabel('CPU (%)')
        #set(gca,'FontSize',14);
        #plt.box(on=True)
        plt.show() # force plot now and then return to main code

    @staticmethod
    def plot_gain(archive_K):
        """ Plot the gain. """
        plt.xlabel('time interval')
        plt.ylabel('gain')
        plt.plot(archive_K, '-b*')
        #set(gca,'FontSize',12);
        #plt.box(on=True)
        plt.show() # force plot now and then return to main code

    def calc_rms_error(self, archive_u, archive_a, c):
        """ Return RMS error. """
        return np.sqrt(np.linalg.norm(archive_u - c * archive_a)**2 / self.time)


metrics=Metrics(100, 10, 0.8)
x = metrics.calc_mrt_cost(np.array(range(8)).T, np.array(range(8, 16)).T)
metrics.plot_mrt()

Την calc_mrt_cost δες βασικά. Την έχω ξαναγράψει γιατί.

Αναφορικά με τις staticmethods, και να μην τις χρησιμοποιήσεις, δεν έχει καμιά ιδιαίτερη διαφορά. Είναι περισσότερο «declaration of intent». Δίνεις στον άλλο να καταλάβει ότι δεν θα αλλάξεις τίποτα στο internal state της κλάσης/instance.

edit

 

@dr-fuzzy

bonus points τώρα για να κάνεις και ένα vectorized implementation.

edit2

Οι οld style classes χρησιμοποιούν different semantics στην κληρονομικότητα (MRO, multiple inheritance κτλ). Εσύ να χρησιμοποιείς πάντα new style, δηλαδή να φροντίζεις η υπερκλάση όλων των κλάσεών σου να είναι η object. Δες εδω πιο αναλυτικά: http://stackoverflow.com/questions/54867/what-is-the-difference-between-old-style-and-new-style-classes-in-python

 

assignment εννοώ αυτό

def foo(self, param1, param2):
    self.__param1 = param1      # this is assignment
    self.__param2 = param2      # this is assignment
    return self.__param1 + self.__param2

Με τον τρόπο που το κάνεις, είναι περιττό.

def foo(self, param1, param2):
    return param1 + param2

[iceblade]

Γενικά, ό,τι είπε ο pmav99. Επιπλέον να πω τα εξής:

Δεν υπάρχει κανένας λόγος να χρησιμοποιήσεις classes για τη δουλειά που θέλεις να κάνεις. Classes και OOP μας δίνουν ωραία πράγματα (polymorphism, inheritance, encapsulation) από τα οποία όμως δε χρειάζεσαι τίποτα. Αντί για αυτό ένα dictionary για ομαδοποίηση των μεταβλητών και functions είναι υπερ αρκετό. Γενικά όταν φτιάχνεις μια κλάση της οποίας τα methods υλοποιούνται ως επί το πλείστον ως static, αυτό το class δεν έχει κανένα νόημα. Για περισσότερα δες εδώ.

Επίσης, εν θες να μαρκάρεις κάποιες μεταβλητές ως private να χρησιμοποιείς single underscore (_param1) και όχι double underscore (__param1). Το single underscore είναι το κοινώς αποδεκτό convention στην Python για private variables, ενώ το double underscore χρησιμοποιείται από το name mangling και κυρίως για να εμποδίζει methods σε derived classes να κάνουν override methods από τα parent classes τους. Για περισσότερα εδώ.

[Dr.Fuzzy]

@pmav99

• Ωραίος, τέτοια πραγματάκια είχα στο μυαλό μου να δω βασικά όταν άνοιξα το θέμα. Θα φροντίσω να τα υιοθετήσω και στις υπόλοιπες κλάσεις.
• Ευχαριστώ για το rewrite της calc_mrt_cost.  Ειδικά το for i, (a, D) in enumerate(zip(archive_a, archive_D)): ... ... implementation είναι πολύ πιο ψαγμένο και καθαρό από το δικό μου (μάλλον φταίει ότι πρακτικά το έγραψα όπως το είχα στη MATLAB και όχι in a Pythonic way)
• Στη main class (βασικά αυτή είναι η super class...) που υλοποιούνται διάφορα estimator models και μας ενδιαφέρει το computation performance ο κώδικας είναι vectorized. 
• assignment..οντως είναι περιττός ο τρόπος που κάνω τις αναθέσεις.
• Old-style, new style classes...Ι'd never would have known, if you haven't mentioned it!

@iceblade

• Αρχικά το είχα self.time (όπως το έγραψε ο pmav99 τώρα) μετά κάπου διάβασα ότι 1 _ είναι semi-private και 2 __ private και το άλλαξα (συγκεριμένα δες #62 εδώ http://stackoverflow.com/questions/1301346/the-meaning-of-a-single-and-a-double-underscore-before-an-object-name-in-python).
• Το project αποτελείται και από άλλες κλάσεις όπου οι μέθοδοι δεν είναι static. Άλλωστε και εδώ static είναι τα plot methods μόνο.
• O κώδικας όπως τον έκανα αρχικά port από το MATLAB ήταν pure functions χωρίς OOP. Επειδή όμως πρόκειται να επεκταθεί αρκετά και να συνδεθεί και με άλλα frameworks, πχ, RUBIS, XEN Python wrappers, κλπ κλπ, θεώρησα καλό να γραφτεί OOP κυρίως για καλύτερο code reuse, maintenance και library adaptation.
• To dictionary που πρότεινε ο groot θα το χρησιμοποιήσω στα plot.

 

Έχει νόημα τελικά να έχω separate classes Metrics και PlotMetrics (ή MetricsPlot...;;; ) όπως προτάθηκε από τον mad-professor;

[iceblade]

@iceblade

• Αρχικά το είχα self.time (όπως το έγραψε ο pmav99 τώρα) μετά κάπου διάβασα ότι 1 _ είναι semi-private και 2 __ private και το άλλαξα (συγκεριμένα δες #62 εδώ http://stackoverflow.com/questions/1301346/the-meaning-of-a-single-and-a-double-underscore-before-an-object-name-in-python).

 

Το post που έχεις κάνει quote αναφέρεται σε documentation της Python που είναι παλιό. Στην Python δεν υπάρχει "semi-private" και ούτε φυσικά "πραγματικά" private variables (μπορείς να έχεις κανονικά access και στα double underscore variables). Αυτά επαναλαμβάνω δεν είναι ο σκοπός τους να είναι private, το convention είναι το single underscore.

 

Από το επίσημο documentation:

 

“Private” instance variables that cannot be accessed except from inside an object don’t exist in Python. However, there is a convention that is followed by most Python code: a name prefixed with an underscore (e.g. _spam) should be treated as a non-public part of the API (whether it is a function, a method or a data member). It should be considered an implementation detail and subject to change without notice.

 Any identifier of the form __spam (at least two leading underscores, at most one trailing underscore) is textually replaced with _classname__spam, where classname is the current class name with leading underscore(s) stripped. This mangling is done without regard to the syntactic position of the identifier, as long as it occurs within the definition of a class. Name mangling is helpful for letting subclasses override methods without breaking intraclass method calls.

 

To class που είδα γραμμένο πραγματικά δεν έχει λόγο ύπαρξης. Για code reuse, maintenance και library adaptation δεν χρειάζεσαι κλάσεις, τα modules, functions και οι δομές τις Python (dictionaries, lists etc) είναι παραπάνω από αρκετά. Μη γράφεις Python με λογική Java, λάθος που το είχα κάνει και γω γιατί σε Java και C# προγραμμάτιζα πριν έρθω στην Python. Φυσικά σε πολλές άλλες περιπτώσεις (π.χ. στο ORM του Django) οι κλάσεις είναι η σωστή επιλογή για την μοντελοποίση data και behavior. Απλά σου εφιστώ την προσοχή στο ότι πολλοί χρησιμοποιούν κλάσεις ενώ δε χρειάζεται, ιδιαίτερα σε scientific programming και automation χρειάζονται σπάνια.