Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update distributions.py #9

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Davide-Ruffini
Copy link

<Multinomial added

Multinomial added
@gpitt71 gpitt71 requested review from gpitt71 and EdoLu June 14, 2024 13:06
@EdoLu
Copy link
Collaborator

EdoLu commented Jun 22, 2024

Ciao @Davide-Ruffini , @gpitt71 :
Di seguito alcuni miei commenti:

  • Manca la nuova multinomiale nel DIST_DICT contenuto in config.py.
  • Esiste nella distribuzione sottostante di scipy un metodo simil "moment"? E' coerente con skewness e kurtosis che abbiamo aggiunto, i.e. funziona allo stesso modo? Vale la pena aggiungere "var" o esiste già?
  • Metodi par_deductible_adjuster e par_deductible_reverter: rispetto al caso univariato nu potrebbe (e tipicamente dovrebbe) essere un array di dimensione pari al parametro p delle probabilità. Farei alcuni assert e controlli iniziali che le dimensioni di p e nu siano compatibili, inoltre avete controllato che le funzioni si comportano secondo le attese anche in questo caso?
  • I metodi ereditati da _DiscreteDistribution (e da _Distribution) funzionano?

@EdoLu
Copy link
Collaborator

EdoLu commented Jun 22, 2024

Commento aggiuntivo meno importante: di solito le pull request (PR) riguardano più contenuti e non solo un singolo commit.
Questa è la prima volta, e quindi ha senso farlo come prova, però in futuro farei una PR dopo che si hanno implementato più aspetti, ad es. una serie o una famiglia intera di distirbuzioni multivariate.

A presto

@gpitt71
Copy link
Owner

gpitt71 commented Jun 28, 2024

Ciao @Davide-Ruffini @EdoLu,

@Davide-Ruffini grazie ancora per il lavoro che stai facendo. Sembra un buon inizio!

Di seguito i miei commenti:

  • Metodo pgf. Docustring. La documentazione non sembra corretta. La parametrizzazione abk è solo per univariate o mi perdo qualcosa?
  • Metodi par_deductible_adjuster, par_deductible_reverter. Non sono ancora implementati perchè non ancora inclusi aspetti di LossModelling. Corretto? Vale la pena che ritorinino NULL o che non ci siano proprio?
  • Skewness e curtosi non dovrebbero ritornare i risultati delle binomiali marginali? Altrimenti manca forse un po' di coerenza matematica. Dovrebbero forse avere un parametro: marginal_id o cose del genere che l'utente può usare per richiedere risultati per una specifica marginale.

Eventuali aspetti aggiunti dovrebbero essere documentati.

Aspetto un vostro feedback,
Ciao,
Gabriele

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants