[16.0][l10n_it_intrastat_statement] Code optimization when generating the statement #3888

robyf70 · 2024-01-24T15:40:44Z

La PR ottimizza il codice che elabora lo statement dell'intrastat rimuovendo 2 dei 3 for loop che vengono eseguiti per ogni blocco di righe intrastat di account.move rendendo l'algoritmo più efficiente e veloce.

SirAionTech

Grazie della PR!
Ho fatto solo revisione del codice, in effetti qui venivano fatti un po' di loop che si possono evitare.
Puoi riportare qualche dato per avere un'idea di quanto si guadagna con queste modifiche?

SirAionTech · 2024-01-25T11:36:01Z

l10n_it_intrastat_statement/models/intrastat_statement.py

-                        len(statement_data[statement_section_field]) + 1
-                    )
-                    statement_data[statement_section_field].append((0, 0, st_line))
+            section_details = section_field_reverse[inv_intra_line.statement_section]


Se la chiave non dovesse esistere, qui verrebbe sollevato un errore (prima invece si andava semplicemente avanti); potresti gestire il caso in cui la chiave non viene trovata?
Ad esempio potresti usare .get.

Se la chiave non dovesse esistere, qui verrebbe sollevato un errore (prima invece si andava semplicemente avanti); potresti gestire il caso in cui la chiave non viene trovata? Ad esempio potresti usare .get.

Questo non dovrebbe accadere perchè le sezioni hanno un nome ben definito. Infatti il codice che genera il dizionario section_field_reverse serve per questo, e se dovesse succedere allora necessariamente è cambiato qualcosa nel nome delle sezioni e quindi nei campi che le mantengono.

Se la chiave non dovesse esistere, qui verrebbe sollevato un errore (prima invece si andava semplicemente avanti); potresti gestire il caso in cui la chiave non viene trovata? Ad esempio potresti usare .get.

Questo non dovrebbe accadere perchè le sezioni hanno un nome ben definito. Infatti il codice che genera il dizionario section_field_reverse serve per questo, e se dovesse succedere allora necessariamente è cambiato qualcosa nel nome delle sezioni e quindi nei campi che le mantengono.

Certo, non dovrebbe accadere, ma se dovesse accadere il codice precedente non sollevava errore mentre il nuovo codice sì: quindi penso sia meglio gestire questo caso come succedeva prima.

SirAionTech · 2024-01-25T11:43:00Z

l10n_it_intrastat_statement/models/intrastat_statement.py

+                statement_section_field = self.get_section_field_name(
+                    section_type, section_number
+                )
+                statement_data[statement_section_field] = list()


Prima statement_data conteneva la chiave di una sezione solo se la sezione aveva delle righe, ora invece ha a prescindere le chiavi di tutte le sezioni, come mai?
A occhio non credo porti grossi vantaggi: nel caso si potrebbe ripristinare il comportamento precedente, che ne pensi?
Visto che questo dizionario viene poi usato per write, questo potrebbe svuotare le sezioni che non hanno righe invece di lasciarle com'erano (comportamento precedente).

Il comportamento che hai descritto è esattamente il contrario, prima la logica era: creiamo tutte le entry dei campi di tutte e 5 le sezioni purchase e sale per poi fare la write(). La PR cambia il comportamento e và a scrivere solo i campi che sono effettivamente usati lasciando gli altri inalterati

Il comportamento che hai descritto è esattamente il contrario, prima la logica era: creiamo tutte le entry dei campi di tutte e 5 le sezioni purchase e sale per poi fare la write(). La PR cambia il comportamento e và a scrivere solo i campi che sono effettivamente usati lasciando gli altri inalterati

Prima il codice in pratica era:

statement_data = dict() for inv_intra_line in invoices.mapped("intrastat_line_ids"): for section_type in ["purchase", "sale"]: for section_number in range(1, 5): # calcola se c'è una riga `st_line` nella sezione corrente if not st_line: continue statement_data[statement_section_field].append((0, 0, st_line))

Quindi aggiungeva una sezione a statement_data solo quando la sezione conteneva delle righe.

Ora invece è

for section_type in ["purchase", "sale"]: for section_number in range(1, 5): statement_data[statement_section_field] = list()

Quindi aggiunge sempre tutte le sezioni a statement_data.

Poi entrambi fanno self.write(statement_data).
Quindi il nuovo codice rischia di scrivere una lista vuota (quindi cancellare) i valori nelle sezioni che non sono state popolate, questo prima non succedeva.

In realtà arrivano tutti i campi delle sezioni, ma alcuni possono essere vuoti, magari li eliminiamo prima di scrivere a scanso di equivoci ;)

SirAionTech · 2024-01-25T11:46:16Z

l10n_it_intrastat_statement/models/intrastat_statement.py

-                    )
-                    statement_data[statement_section_field].append((0, 0, st_line))
+            section_details = section_field_reverse[inv_intra_line.statement_section]
+            statement_section_model_name = self.get_section_model(*section_details)


Sempre in ambito ottimizzazione: si potrebbe evitare di chiamare get_section_model e get_section_field_name per ogni riga, che ne pensi?
Il risultato potrebbe essere già nel dizionario section_field_reverse, invece dei section_details che vengono usati solo per chiamare questi due metodi.

Si! Ovviamente è possibile farlo. Tra l'altro era quello che ho pensato di fare inizialmente, ma poi ho optato di riutilizzare le chiamate già esistenti. Cambio la PR e vediamo

SirAionTech

Ottimizzazione senza avere dei numeri sottomano potrebbe non essere davvero un'ottimizzazione, per questo avevo chiesto

Puoi riportare qualche dato per avere un'idea di quanto si guadagna con queste modifiche?

SirAionTech · 2024-03-20T14:17:38Z

l10n_it_intrastat_statement/models/intrastat_statement.py

+                    "field": self.get_section_field_name(
+                        section_type=section_type, section_number=section_number
+                    ),


Suggested change

"field": self.get_section_field_name(

section_type=section_type, section_number=section_number

),

"field": statement_section_field,

Abbiamo già questo valore, non serve chiamare di nuovo il metodo che lo calcola.

SirAionTech · 2024-03-20T14:19:55Z

l10n_it_intrastat_statement/models/intrastat_statement.py

+                )
+                statement_data[statement_section_field] = list()
+                section_model = self.get_section_model(
+                    section_type=section_type, section_number=section_number


Suggested change

section_type=section_type, section_number=section_number

section_type, section_number

Questi non sono kwargs ma args, quindi si possono assegnare semplicemente in base alla loro posizione.

ok! Lo cambio

SirAionTech · 2024-03-20T14:34:15Z

l10n_it_intrastat_statement/models/intrastat_statement.py

-                    )
-                    statement_data[statement_section_field].append((0, 0, st_line))
+            section_details = section_field_reverse[inv_intra_line.statement_section]
+            statement_section_model_name = self.get_section_model(*section_details)


SirAionTech · 2024-03-20T14:39:33Z

l10n_it_intrastat_statement/models/intrastat_statement.py

+        statement_data = {}
+        section_field_reverse = {}
+        for section_type in ["purchase", "sale"]:
+            for section_number in range(1, 5):
+                statement_section_field = self.get_section_field_name(
+                    section_type, section_number
+                )
+                statement_data[statement_section_field] = list()


Suggested change

statement_data = {}

section_field_reverse = {}

for section_type in ["purchase", "sale"]:

for section_number in range(1, 5):

statement_section_field = self.get_section_field_name(

section_type, section_number

)

statement_data[statement_section_field] = list()

statement_data = defaultdict(list)

section_field_reverse = {}

for section_type in ["purchase", "sale"]:

for section_number in range(1, 5):

statement_section_field = self.get_section_field_name(

section_type, section_number

)

importando prima from collections import defaultdict.
In questo modo torna il comportamento precedente (di cui abbiamo già parlato in https://github.com/OCA/l10n-italy/pull/3888/files#r1466248365) di valorizzare solo i campi che avranno delle righe, ed evitiamo di creare delle liste vuote e doverle poi rimuovere più sotto nella dictionary comprehension (quindi la write potrà tornare com'era prima).
Cosa ne pensi?

Ok! Lo cambio

robyf70 · 2024-03-20T15:11:38Z

Ottimizzazione senza avere dei numeri sottomano potrebbe non essere davvero un'ottimizzazione, per questo avevo chiesto

Puoi riportare qualche dato per avere un'idea di quanto si guadagna con queste modifiche?

L'algoritmo precedende partendo dal secondo for abbiamo 2 iterazioni fisse (sale e purchase), il terzo for è sempre fisso a * 5 (il range(1, 5) ) quindi in totale abbiamo:

len(invoices.mapped("intrastat_line_ids")) * 2 * 5

contro quello della PR

len(invoices.mapped("intrastat_line_ids"))

Quindi la PR riduce a 1/10 le iterazioni rispetto al vecchio codice rendendolo lineare. A questo dobbiamo sommare la costruzione del mapping per tutte le sezioni di sale e purchase prima del for lineare

…essary triple for loop when generating the statement

SirAionTech · 2024-03-20T15:34:37Z

Ottimizzazione senza avere dei numeri sottomano potrebbe non essere davvero un'ottimizzazione, per questo avevo chiesto

Puoi riportare qualche dato per avere un'idea di quanto si guadagna con queste modifiche?

Ottimizzazione senza avere dei numeri sottomano potrebbe non essere davvero un'ottimizzazione, per questo avevo chiesto

Puoi riportare qualche dato per avere un'idea di quanto si guadagna con queste modifiche?

L'algoritmo precedende partendo dal secondo for abbiamo 2 iterazioni fisse (sale e purchase), il terzo for è sempre fisso a * 5 (il range(1, 5) ) quindi in totale abbiamo:

len(invoices.mapped("intrastat_line_ids")) * 2 * 5

contro quello della PR

len(invoices.mapped("intrastat_line_ids"))

Quindi la PR riduce a 1/10 le iterazioni rispetto al vecchio codice rendendolo lineare. A questo dobbiamo sommare la costruzione del mapping per tutte le sezioni di sale e purchase prima del for lineare

Grazie, questo in teoria, ma in pratica hai provato quanto si guadagna?
Ad esempio, per generare una dichiarazione di 1000 righe prima ci volevano 10 secondi e ora invece ce ne vuole 1

robyf70 · 2024-03-20T15:44:55Z

Ottimizzazione senza avere dei numeri sottomano potrebbe non essere davvero un'ottimizzazione, per questo avevo chiesto

Puoi riportare qualche dato per avere un'idea di quanto si guadagna con queste modifiche?

Ottimizzazione senza avere dei numeri sottomano potrebbe non essere davvero un'ottimizzazione, per questo avevo chiesto

Puoi riportare qualche dato per avere un'idea di quanto si guadagna con queste modifiche?

L'algoritmo precedende partendo dal secondo for abbiamo 2 iterazioni fisse (sale e purchase), il terzo for è sempre fisso a * 5 (il range(1, 5) ) quindi in totale abbiamo:
len(invoices.mapped("intrastat_line_ids")) * 2 * 5
contro quello della PR
len(invoices.mapped("intrastat_line_ids"))
Quindi la PR riduce a 1/10 le iterazioni rispetto al vecchio codice rendendolo lineare. A questo dobbiamo sommare la costruzione del mapping per tutte le sezioni di sale e purchase prima del for lineare

Grazie, questo in teoria, ma in pratica hai provato quanto si guadagna? Ad esempio, per generare una dichiarazione di 1000 righe prima ci volevano 10 secondi e ora invece ce ne vuole 1

Un test sistematico di questo tipo proprio no, mi sono limitato a rimuovere i 2 loop interni che da soli multiplicavano per 10 il numero di iterazioni per un algoritmo che avrebbe essere lineare.

github-actions · 2024-08-18T12:33:12Z

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

robyf70 mentioned this pull request Jan 24, 2024

Intrastat Code Optimization #3886

Closed

2 tasks

francesco-ooops linked an issue Jan 24, 2024 that may be closed by this pull request

Intrastat Code Optimization #3886

Closed

2 tasks

SirAionTech requested changes Jan 25, 2024

View reviewed changes

robyf70 force-pushed the 16.0-l10n_it_intrastat_statement-code_optimization branch 2 times, most recently from b2910f0 to 4758d67 Compare March 18, 2024 15:45

robyf70 requested a review from SirAionTech March 18, 2024 16:12

SirAionTech requested changes Mar 20, 2024

View reviewed changes

[16.0][l10n_it_intrastat_statement] Code optimization to remove unnec…

2b2d102

…essary triple for loop when generating the statement

robyf70 force-pushed the 16.0-l10n_it_intrastat_statement-code_optimization branch from 4758d67 to 2b2d102 Compare March 20, 2024 15:21

github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Aug 18, 2024

github-actions bot closed this Sep 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[16.0][l10n_it_intrastat_statement] Code optimization when generating the statement #3888

[16.0][l10n_it_intrastat_statement] Code optimization when generating the statement #3888

robyf70 commented Jan 24, 2024

SirAionTech left a comment

SirAionTech Jan 25, 2024

robyf70 Mar 18, 2024

SirAionTech Mar 18, 2024

SirAionTech Jan 25, 2024

robyf70 Mar 18, 2024

SirAionTech Mar 18, 2024

robyf70 Mar 18, 2024

SirAionTech Jan 25, 2024

robyf70 Mar 18, 2024

SirAionTech Mar 20, 2024

SirAionTech left a comment

SirAionTech Mar 20, 2024

SirAionTech Mar 20, 2024

robyf70 Mar 20, 2024

SirAionTech Mar 20, 2024

SirAionTech Mar 20, 2024

robyf70 Mar 20, 2024

robyf70 commented Mar 20, 2024

SirAionTech commented Mar 20, 2024

robyf70 commented Mar 20, 2024

github-actions bot commented Aug 18, 2024

	section_type=section_type, section_number=section_number
	section_type, section_number

[16.0][l10n_it_intrastat_statement] Code optimization when generating the statement #3888

[16.0][l10n_it_intrastat_statement] Code optimization when generating the statement #3888

Conversation

robyf70 commented Jan 24, 2024

SirAionTech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SirAionTech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robyf70 commented Mar 20, 2024

SirAionTech commented Mar 20, 2024

robyf70 commented Mar 20, 2024

github-actions bot commented Aug 18, 2024