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

JOSS review : Software paper #15

Open
11 of 13 tasks
couasnonanais opened this issue Jun 25, 2024 · 1 comment
Open
11 of 13 tasks

JOSS review : Software paper #15

couasnonanais opened this issue Jun 25, 2024 · 1 comment

Comments

@couasnonanais
Copy link

couasnonanais commented Jun 25, 2024

Dear authors of the VineCopulas package and @judithclaassen ,

Apologies for the delay in my review. I very much enjoyed having a look at the package and I think it is a great contribution to the Python community. You will find below some comments concerning the paper on top of the ones mentioned by @haozcbnu . I will be opening a separate issue for the documentation.

Concerning Figure 1: I think that this figure summarizes very well the main functionalities of the package and therefore I would suggest to clarify the figure.

  • It is not obvious at first that the letters in the caption refer to the arrows.
  • In the Table with the Data, I would suggest to change the notation to be in line with common mathematical notations. Therefore, I would add next to Variable 1 ($X_1$), etc. In the table, the notation then becomes $x^1_1$ until $x^n_1$ and for $X_2$: $x^1_2$ until $x^n_2$. Calling this table as Table 1 data or somehting like that can then clarify the caption (see next point)
  • In the caption after (A), you could then mention that "Samples from Table 1 - data, consisting of both continuous and discrete varibles (plotted in blue) are transformed into pseudo-observations using their marginal distributions (shown in green)". It would be good to mention pseudo-observations/pseudo-data there too (I provide a suggestion but feel free to use it or not)

Concerning the Statement of need:

  • It is not clear who the target audience is. I understand after reading the text that one type of audience is Python users and from the examples, someone who has some mathematical background. You could add a few sentences about this in the paper to explicitely adress this (since this is in the review checklist)
  • I understand that the paragraph between line 49 and 53 lists the unique features of the package. I would make this more explicit. (for e.g. "The unique features of this package is that is integrates....") or even using a bullet point list. Here I would clarify "fitting CDF and PDF" given the question you received as an issue (issue PDF and CDF calculation! #14 )
  • You could then extend this bullet list to the main functionalities of the package (as highlighted in the examples)

Concerning references:

  • I would add all references that are mentioned in the example Python notebooks as I would argue that they are key references.
  • In the Statement of need, I would highlight the paragraph

Typos etc:

  • line 50: CDF --> you mean cumulative here, correct?
  • line 51: random sample generation. (sampling --> sample)
  • line 58: Python
  • line 54: reference missing for the copulas package?
  • The last sentences reads a bit strange because these studies have used alternative coding languages or Python package. Maybe rephrase to put the emphasis on the need for more complete package to apply for these high dimensionality and complex problem. But that also can welcome contributions from the growing Python community.
@couasnonanais
Copy link
Author

I forgot to mention that it would be good to mention or point the user towards the list of univariate distributions available for the fit of the marginal distributions. And explicitely mention the scipy package.

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

No branches or pull requests

1 participant