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 -- A list of minor comments #163

Closed
7 tasks done
Farnazmdi opened this issue Jan 31, 2023 · 8 comments · Fixed by #165
Closed
7 tasks done

JOSS Review -- A list of minor comments #163

Farnazmdi opened this issue Jan 31, 2023 · 8 comments · Fixed by #165
Assignees
Labels
documentation Software and example documentation

Comments

@Farnazmdi
Copy link

Farnazmdi commented Jan 31, 2023

Congratulations on developing this package, it is very well written and super useful in its relevant field. I just have a few minor comments.

In the manuscript:

  • line 44, "from" is repeated.
  • line 19 "partioning" is a typo.
  • in figure 1a, the word "Transcription fixed delay" has a typo.
  • It would be nice to accompany the figure1b with a mathematical equation to clarify if there is a simple closed form expression for it.

In the repository:

  • I see that some of the functions have extensive documentation, but it would be clearer if all functions had this, especially specifying the type of the input arguments, such as 'int', 'float', etc. would help with readability of the functions.
  • This one is just a suggestion and mostly my curiosity to see how does Numpyro perform versus your implementation of MCMC inference in Cython. Do you have an idea about this?
  • It would be nice to add a link or some explanation on how to install jupyter notebook kernels in the installation guide. In case somenone wants to clone your repository I guess it would be necessary to install a kernel.
@ayush9pandey
Copy link
Collaborator

ayush9pandey commented Jan 31, 2023

Thanks @Farnazmdi for reviewing bioscrape! I will work on fixing these.
Just want to point you to the latest paper PDF https://raw.githubusercontent.com/openjournals/joss-papers/joss.05057/joss.05057/10.21105.joss.05057.pdf
(also available on the review thread). The update after the previous review fixed the typo error so I have marked that task as done.

sorry the other typo is still there.

@ayush9pandey ayush9pandey self-assigned this Jan 31, 2023
@ayush9pandey ayush9pandey added the documentation Software and example documentation label Jan 31, 2023
@ayush9pandey
Copy link
Collaborator

  • Added instructions about Jupyter notebooks in the Installation page on the Wiki.

@ayush9pandey
Copy link
Collaborator

  • Added docstrings and types of arguments to all user interface functions. We understand that a detailed documentation of the code is an important software engineering practice. The current status of the documentation is in no way complete or up-to industry standards. So, we will keep improving the documentation of the package.
  • For now, I have added a detailed docstring and argument types to all four key functionalities of bioscrape: model construction using Model class constructor, all user-level analysis functions in the analysis module, the inference module's user interface function py_inference, the simulation function py_simulate_model.

All of these changes are on the documentation branch that will be merged soon!

@ayush9pandey
Copy link
Collaborator

@ayush9pandey
Copy link
Collaborator

  • For Figure 1b, since we also have a delay in the model, there is no "nice looking" closed form expression for the model. The delay for each reaction in bioscrape is a probabilistic delay in firing of propensity of that reaction. So, I'm not sure if there is a good closed form expression to write it.

@ayush9pandey
Copy link
Collaborator

  • Fixed paper typos in the joss-paper branch.

@ayush9pandey
Copy link
Collaborator

Hi @Farnazmdi ,

Thanks for reviewing bioscrape. Please let me know if the fixes that I have described are satisfactory and if you have other comments.

@ayush9pandey ayush9pandey linked a pull request Feb 19, 2023 that will close this issue
@Farnazmdi
Copy link
Author

Hi @ayush9pandey Thanks for addressing the comments. I believe these changes resolve all my concerns. Wonderful job!

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

Successfully merging a pull request may close this issue.

2 participants