openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
710 stars 38 forks source link

[REVIEW]: GRAPL: A computational library for nonparametric structural causal modelling, analysis and inference #4534

Closed editorialbot closed 2 years ago

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@max-little<!--end-author-handle-- (Max A. Little) Repository: https://github.com/max-little/GRAPL Branch with paper.md (empty if default branch): Version: v1.4.0 Editor: !--editor-->@Nikoleta-v3<!--end-editor-- Reviewers: @Athene-ai, @fAndreuzzi Archive: 10.5281/zenodo.6959433

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/ff5f7d36e6af106d8708c42d4a095124"><img src="https://joss.theoj.org/papers/ff5f7d36e6af106d8708c42d4a095124/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/ff5f7d36e6af106d8708c42d4a095124/status.svg)](https://joss.theoj.org/papers/ff5f7d36e6af106d8708c42d4a095124)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@Athene-ai & @fAndreuzzi, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @Nikoleta-v3 know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Checklists

πŸ“ Checklist for @Athene-ai

πŸ“ Checklist for @fAndreuzzi

editorialbot commented 2 years ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

For a list of things I can do to help you, just type:

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf
editorialbot commented 2 years ago
Software report:

github.com/AlDanial/cloc v 1.88  T=0.06 s (339.4 files/s, 115343.9 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          17           1032           1630           4203
Markdown                         3             42              0            163
TeX                              1              8              0             59
-------------------------------------------------------------------------------
SUM:                            21           1082           1630           4425
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 2 years ago

Wordcount for paper.md is 802

editorialbot commented 2 years ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1017/S0266466603004109 is OK
- 10.1145/3501714.3501743 is OK
- 10.3389/fnagi.2020.553635 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

Nikoleta-v3 commented 2 years ago

πŸ‘‹πŸΌ @max-little @Athene-ai, @fAndreuzzi this is the review thread for the paper. All of our communications will happen here from now on.

As a reviewer, the first step is to create a checklist for your review by entering

@editorialbot generate my checklist

as the top of a new comment in this thread.

These checklists contain the JOSS requirements βœ… As you go over the submission, please check any items that you feel have been satisfied. The first comment in this thread also contains links to the JOSS reviewer guidelines.

The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention openjournals/joss-reviews#4534 so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

We aim for reviews to be completed within about 2-4 weeks. Please let me know if any of you require some more time. We can also use EditorialBot (our bot) to set automatic reminders if you know you'll be away for a known period of time.

Please feel free to ping me (@Nikoleta-v3) if you have any questions/concerns.

fandreuz commented 2 years ago

Review checklist for @fAndreuzzi

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

fandreuz commented 2 years ago

Hi @max-little, I'm going to help with the review for this paper. I've already submitted some suggestions to your repository such that it's easier for you to track what needs to be changed. For now it's only code structure/tooling, I'll review the actual code later.

I see that your code is not on PyPI, do you plan to upload it there such that your users can install GRAPL via pip install grapl?

fandreuz commented 2 years ago

Hi @max-little, I started looking at your code and I would say that you need to invest some time in documentation. I'm not going to ask full docstrings on every single entity, but there are some methods where docstrings are definitely needed:

Also, I would strongly suggest to prepend an underscore (_) to each method/function that is not intended to be used by users (i.e. private method/for internal usage) as discussed in PEP8.

Suggestions for docstrings from NumPy.

Let me know if you need clarification on any of my points.

max-little commented 2 years ago

Hi Francesco

Thanks for the comments. Now done. Specifically, I've provided more details for the more complex algorithms/functions in the package. For the rest, the existing comments are either self-explanatory, and/or the comments within the code are sufficiently explanatory in order for the functionality to be clear. Again, this follows from literate coding principles (which do clash somewhat with the concept of docstrings).

The other consideration here which I should raise, is that this package has a specific context, that is, it is aimed at those with a background in structural causal inference, and the terminology (hence, variable/parameter naming) is geared towards that community's standards. So, the package is not intended as a substitute for the relevant textbooks (e.g. Pearl's "Causality").

Re: (_) convention, there are no functions/methods in this package which are not intended to be used.

All the best Max

max-little commented 2 years ago

Hi Francesco

Do you have any further points to raise?

Best Max

On Tue, 5 Jul 2022 at 23:42, Francesco Andreuzzi @.***> wrote:

Hi @max-little https://github.com/max-little, I started looking at your code and I would say that you need to invest a little bit of time in documentation. I'm not going to ask full docstrings on every single method, but I would say that there are some method where docstrings would are strongly needed:

  • Public methods (i.e. methods that are intended to be used by the user, like GraplDSL.readgrapl: Even if you feel like you would write obvious things it's good practice to spend some words on explaining what's going to happen and how the parameters are going to be used.
  • Methods in algorithms.py: What are X and Y? simplifywhat? (I'm referring todagfactor`). I feel that I should not need to read a paper to understand the symbols you're using, so docstring here would be very appreciated.

Also, I would strongly suggest to prepend an underscore (_) to each method/function that is not intended to be used by users PEP8 https://peps.python.org/pep-0008/#descriptive-naming-styles.

Suggestions for docstrings from NumPy https://numpydoc.readthedocs.io/en/latest/format.html.

Let me know if you need clarification on any of my points.

β€” Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/4534#issuecomment-1175573632, or unsubscribe https://github.com/notifications/unsubscribe-auth/AYOOCSB4H2SHNXOECWYFYDTVSS26VANCNFSM52NABAUA . You are receiving this because you were mentioned.Message ID: @.***>

-- Max Little (www.maxlittle.net) Associate Professor, University of Birmingham Turing Fellow, Alan Turing Institute TED Fellow (fellows.ted.com/profiles/max-little) Visiting Associate Professor, MIT Author: Machine Learning for Signal Processing, Oxford University Press global.oup.com/academic/product/machine-learning-for-signal-processing-9780198714934 Room 138, School of Computer Science University of Birmingham Birmingham B15 2TT UK +44 7710 609564 Skype: dr.max.little

fandreuz commented 2 years ago

@max-little I've ticked all the points for the paper, I'm going to review the code and try code samples in the next few days hopefully.

For the rest, the existing comments are either self-explanatory, and/or the comments within the code are sufficiently explanatory in order for the functionality to be clear.

I get your point, but this implies that your users need to look at your code in order to find:

  1. Which features are available, besides what you show in the tutorials
  2. What is the meaning of the parameters, what is the expected type for each parameter, etc etc

I guess it's rare to find users willing (or happy) to jump into half an hour of diving into the source code to get to use an open source package. For me, I would definitely, but I try not to assume this when I write open source software.

Again, this follows from literate coding principles (which do clash somewhat with the concept of docstrings).

Docstrings integrate nicely with the Python environment (e.g. IDE) and most importantly enable using tools like Sphinx which auto-generate documentation pages in HTML format. For me the initial effort you require to your users is quite a lot, but I'm going to wait for the opinion of @Athene-ai on this topic.

max-little commented 2 years ago

Hi Francesco

Thanks for your reply, some comments below:

On Fri, 8 Jul 2022 at 00:30, Francesco Andreuzzi @.***> wrote:

@max-little https://github.com/max-little I've ticked all the points for the paper, I'm going to review the code and try code samples in the next few days hopefully.

For the rest, the existing comments are either self-explanatory, and/or the comments within the code are sufficiently explanatory in order for the functionality to be clear.

I get your point, but this implies that your users need to look at your code in order to find:

  1. Which features are available, besides what you show in the tutorials

Well no, an IDE like VSCode automatically scans the code, clearly showing you what features/methods are available, you get a nice list of functions in a side panel. You can also just type help(grapl.algorithms) in an interactive Python shell, and get a clean list of the functionality, because minimal docstrings are provided for all features.

  1. What is the meaning of the parameters, what is the expected type for each parameter, etc etc

But you can generally infer types from (a) the defaults, or (b) the name of the parameter, or (c) the docstring. Detailed type-level declarations would really clutter the code. For instance, with a function like admg.de, typing help(grapl.admg.de) gives you:

de(self, nodes) | Find the union of all descendants of a given set of nodes, and the nodes themselves.

So it's entirely self-explanatory: the function admg.de requires a node parameter, which is a set of nodes. It's really as simple as that.

I guess it's rare to find users willing (or happy) to jump into half an hour of diving into the source code to get to use an open source package. For me, I would definitely, but I try not to assume this when I write open source software.

As I said, I think you are missing the context. This is research-level software, it isn't intended to be used by those who have no background at all in the topic. As such, providing extensive docstrings with content down to the type declaration level, just obscures the code making it harder to understand. The amount of comments would dwarf the actual code in size.

Again, this follows from literate coding principles (which do clash somewhat with the concept of docstrings).

Docstrings integrate nicely with the Python environment (e.g. IDE) and most importantly enable using tools like Sphinx https://www.sphinx-doc.org/en/master/ which auto-generate documentation pages in HTML format. For me the initial effort you require to your users is quite a lot, but I'm going to wait for the opinion of @Athene-ai https://github.com/Athene-ai on this topic.

That's not really the point: literate coding is about creating "self-documenting" code, to the extent that is possible. I agree that integrating with tools like IDEs and document generators such as Sphinx, provides a nice level of convenience with "referencing automation", but this can't be a substitute for understanding the relevant concepts. That would be a different kind of package.

Once again, thanks for your comments/contributions, it's helpful to have these discussions.

Best Max

-- Max Little (www.maxlittle.net) Associate Professor, University of Birmingham Turing Fellow, Alan Turing Institute TED Fellow (fellows.ted.com/profiles/max-little) Visiting Associate Professor, MIT Author: Machine Learning for Signal Processing, Oxford University Press global.oup.com/academic/product/machine-learning-for-signal-processing-9780198714934 Room 138, School of Computer Science University of Birmingham Birmingham B15 2TT UK +44 7710 609564 Skype: dr.max.little

fandreuz commented 2 years ago

Dear @max-little,

As I said, I think you are missing the context. This is research-level software, it isn't intended to be used by those who have no background at all in the topic.

I expect some users would be blocked by lack of understanding in Python programming rather than in the topic GRAPL addresses. Research-level software means that if I'm a C++ programmer it should not take me too much time to start using what you provide. Or at least this is what I would expect from a JOSS publication.

As such, providing extensive docstrings with content down to the type declaration level, just obscures the code making it harder to understand.

I would suggest taking a look at some other open source projects published in order to see if the kind of documentation you provide is similar to what other authors are doing. I took some links from the first 4 papers I found on the main page of JOSS:

As you can see they all provide extensive explanations and examples, moreover in the Module index detailed descriptions (with parameters and return value) are provided for a big subset of the functions provided by the package. The last one is the package which provides the least complete documentation, but as you can see in the file I linked for you the author provided very detailed docstrings for all the public functions, to be used directly by the user.

Clearly my survey is not complete and depending on the reviewer you may find different levels of documentation accepted for JOSS publications. This is why I said I'd like to wait for my fellow reviewer before asking additional improvements on documentation.

max-little commented 2 years ago

Hi Francesco

As I said, I think you are missing the context. This is research-level software, it isn't intended to be used by those who have no background at all in the topic.

I expect some users would be blocked by lack of understanding in Python programming rather than in the topic GRAPL addresses. Research-level software means that if I'm a C++ programmer it should not take me too much time to start using what you provide. Or at least this is what I would expect from a JOSS publication.

I don't get that, to be honest. I think that, if the code is well-structured, you shouldn't have that much difficulty in going from e.g. Python to C++ as they are both imperative languages, albeit with their own idiosyncrasies, although I agree you would struggle if it were written in Haskell and you had no background in functional programming at all. I think if you are asking code to be written such that someone who has no knowledge of the language at all of the language in which it is written, I don't really see how they can use it without getting lost anyway. In any case, I'm just going on JOSS's own description:

"JOSS publishes articles about research software. This definition includes software that: solves complex modeling problems in a scientific context (physics, mathematics, biology, medicine, social science, neuroscience, engineering); supports the functioning of research instruments or the execution of research experiments; extracts knowledge from large data sets; offers a mathematical library; or similar.

JOSS submissions must:

It doesn't mention anything about how much time it takes a user to pick up the software, or the level of expected expertise in the topic, and I assume this is for good reason because some topics are more complex and require extensive background knowledge, so the package would, presumably require a lot more time to pick up. Others are just more generic because they are less specialist (see below for more on this).

As such, providing extensive docstrings with content down to the type declaration level, just obscures the code making it harder to understand.

I would suggest taking a look at some other open source projects published in order to see if the kind of documentation you provide is similar to what other authors are doing. I took some links from the first 4 papers I found on the main page of JOSS:

https://lanl.github.io/spiner/main/index.html https://rbturnbull.github.io/ausdex/ https://python-kmedoids.readthedocs.io/en/latest/ https://github.com/nasa/simupy-flight/blob/master/simupy_flight/__init__.py

As you can see they all provide extensive explanations and examples, moreover in the Module index detailed descriptions (with parameters and return value) are provided for a big subset of the functions provided by the package. The last one is the package which provides the least complete documentation, but as you can see in the file I linked for you the author provided very detailed docstrings for all the public functions, to be used directly by the user.

Clearly my survey is not complete and depending on the reviewer you may find different levels of documentation accepted for JOSS publications. This is why I said I'd like to wait for my fellow reviewer before asking additional improvements on documentation.

I did already take quite a long look at other repositories before preparing the code for JOSS. Your informal survey has picked out much more "generic" software packages. By contrast a package which is much more "domain specific"/specialist topic, such as this one: https://gitlab.com/thartwig/asloth

has a lot less comment detail than GRAPL, e.g.: https://gitlab.com/thartwig/asloth/-/blob/main/scripts/plot_asloth.py https://gitlab.com/thartwig/asloth/-/blob/main/src/SF_routines.F90

I don't know the topic of that package myself, but there must be a reason why the code is not cluttered with extraneous comments; I would suggest that is because, like GRAPL, this is intended for specialists who can read the code and understand what the software is doing because it is, effectively presented in a "language" they understand already.

Hope that helps explain what I mean more clearly.

All the best Max

β€” Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

-- Max Little (www.maxlittle.net) Associate Professor, University of Birmingham Turing Fellow, Alan Turing Institute TED Fellow (fellows.ted.com/profiles/max-little) Visiting Associate Professor, MIT Author: Machine Learning for Signal Processing, Oxford University Press global.oup.com/academic/product/machine-learning-for-signal-processing-9780198714934 Room 138, School of Computer Science University of Birmingham Birmingham B15 2TT UK +44 7710 609564 Skype: dr.max.little

max-little commented 2 years ago

Another relevant example package would be this one: https://gitlab.ruhr-uni-bochum.de/reichp2y/proppy

No docstrings at all on some of the major entry points (e.g. see simulation.run_simulation): https://gitlab.ruhr-uni-bochum.de/reichp2y/proppy/-/blob/master/proppy/simulation.py https://gitlab.ruhr-uni-bochum.de/reichp2y/proppy/-/blob/master/proppy/source.py

There is additional documentation, but it's generated automatically from the code, so it's no more detailed than the docstrings provided.

Again, I presume this is because, the code is largely self-explanatory.

Best Max

Athene-ai commented 2 years ago

Hi all,

I think that it is quite tricky to understand how to move from Phyton to C++.

Thus could be better explained

On Friday, 8 July 2022, max-little @.***> wrote:

Another relevant example package would be this one: https://gitlab.ruhr-uni-bochum.de/reichp2y/proppy

No docstrings at all on some of the major entry points (e.g. see simulation.run_simulation): https://gitlab.ruhr-uni-bochum.de/reichp2y/proppy/-/ blob/master/proppy/simulation.py https://gitlab.ruhr-uni-bochum.de/reichp2y/proppy/-/ blob/master/proppy/source.py

There is additional documentation, but it's generated automatically from the code, so it's no more detailed than the docstrings provided.

Again, I presume this is because, the code is largely self-explanatory.

Best Max

β€” Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/4534#issuecomment-1178448969, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANGYEVBNFEVSS62CHQJQCELVS6CU7ANCNFSM52NABAUA . You are receiving this because you were mentioned.Message ID: @.***>

-- Daniela Cialfi, PhD


Postdoctoral researcher University "G.D'Annunzio" - Chieti Pescara Viale Pindaro 42 - 65127 Pescara ( Italy ) Tel. +39 3930571128 Email: @.*** < https://webmail.unich.it/horde/imp/message.php?mailbox=INBOX.sent-mail&index=37#>

max-little commented 2 years ago

OK, thanks both. So, it looks like the consensus is you would be satisfied with: expanded docstrings for most methods/functions? Best, Max

Athene-ai commented 2 years ago

yes [image: Mailtrack] https://mailtrack.io?utm_source=gmail&utm_medium=signature&utm_campaign=signaturevirality11& Sender notified by Mailtrack https://mailtrack.io?utm_source=gmail&utm_medium=signature&utm_campaign=signaturevirality11& 07/08/22, 10:49:54 AM

On Fri, 8 Jul 2022 at 10:46, max-little @.***> wrote:

OK, thanks both. So, it looks like the consensus is you would be satisfied with: expanded docstrings for most methods/functions? Best, Max

β€” Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/4534#issuecomment-1178723766, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANGYEVETEWTSD2JEWFSFPLTVS7TGPANCNFSM52NABAUA . You are receiving this because you were mentioned.Message ID: @.***>

fandreuz commented 2 years ago

So, it looks like the consensus is you would be satisfied with: expanded docstrings for most methods/functions?

Yes, most meaning what is going to be used by the user. You should document also classes, as they support docstrings too.

I would also suggest to split functions into smaller, more maintainable ones, it looks quite weird and dangerous to me that you don't have functions which are not going to be used by the user. Even if you do not share any code among different functions, you should try to pack your logic into smaller, single-responsibility functions.

max-little commented 2 years ago

Hi @fAndreuzzi @Athene-ai (cc @Nikoleta-v3) As requested, I have now made substantial changes as follows:

  1. Extensive updates to docstrings.
  2. Methods, member variables and classes all now fully documented.
  3. Parameter and return types detailed.
  4. Further detail on functionality for all methods.
  5. As per our discussion, on further inspection, there are some internal methods which would be best described as "hidden" i.e. should not be called by the user e.g. as closures (in contradiction to what I was claiming earlier). Therefore, as requested, these methods are labelled with '_' and do not have docstrings (in contrast to the "public" methods), following Python conventions. I think the code is now a lot more clearly presented and described, and should be easier to maintain. Hope this is satisfactory. Best Max
fandreuz commented 2 years ago

Thank youΒ for addressing the problem we raised @max-little, I'm quite busy these days so I'll have a look next week.

max-little commented 2 years ago

Thanks @fAndreuzzi .

fandreuz commented 2 years ago

Hi @max-little, first of all thank you for addressing the issues mentioned by @Athene-ai and me. I ticked some more checkboxes in my review.

Looking at the tutorials, I have some more observations:

Also, I tried to run this snippet:

import grapl.algorithms as algs
import grapl.dsl as dsl

grapl_obj = dsl.GraplDSL()
G = grapl_obj.readgrapl('hello')

and received this error in response:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/francescoandreuzzi/Programming/python/GRAPL/grapl/dsl.py", line 111, in readgrapl
    self.parser.parse(scm_string)
  File "/Users/francescoandreuzzi/Programming/python/GRAPL/grapl/ply/yacc.py", line 333, in parse
    return self.parseopt_notrack(input, lexer, debug, tracking, tokenfunc)
  File "/Users/francescoandreuzzi/Programming/python/GRAPL/grapl/ply/yacc.py", line 1201, in parseopt_notrack
    tok = call_errorfunc(self.errorfunc, errtoken, self)
  File "/Users/francescoandreuzzi/Programming/python/GRAPL/grapl/ply/yacc.py", line 192, in call_errorfunc
    r = errorfunc(token)
  File "/Users/francescoandreuzzi/Programming/python/GRAPL/grapl/dsl.py", line 102, in p_error
    raise SyntaxError("Grapl syntax error at line: " + str(p.lineno) + ", near: " + p.value)
AttributeError: 'NoneType' object has no attribute 'lineno'

I get that hello is definitely not the kind of output that you expect, but I feel like you should catch this mistake from the user before it triggers an exception and inform the user that (maybe) the format they used for the input was not appropriate.

Let me know what you think about this

max-little commented 2 years ago

Thanks @fAndreuzzi . I've merged in your suggested changes to the tutorials and the readme, which look sensible to me. I'll see about adding a short explanatory table for the tutorials.

Re: syntax error, well, obviously, if you just put random text into the DSL parser without understanding the language at all, then you will likely just get lexical, syntax or semantic errors, which would happen with any such domain-specific language (DSL). The DSL code does, however, include complete error handling. This is all pretty standard for any DSL. Unfortunately, in this case you discovered a bug whereby the case of missing input was generating a spurious exception. This is now fixed in the latest version.

To explain in more detail, in your test the string 'hello' on its own doesn't have an end of command (EOC) terminator. In fact, 'hello;' (with the semicolon command terminator) would be a valid GRAPL DSL string, as you can check by running:

import grapl.dsl as dsl grapl_obj = dsl.GraplDSL() G = grapl_obj.readgrapl('hello;')

(What you will get is a graph G with a single node called 'hello', you can infer this from the tutorials and/or see precisely why by looking at the EBNF for the DSL, in dsl.py).

In any case, what you will now get, if you don't terminate a command, is the following:

grapl_obj.readgrapl('hello') Grapl syntax error: unexpected end of input

In general, GRAPL error handling reports the line and some characters near where the error occurred (again standard for this kind of DSL). Furthermore, you can always debug a string using dsl.displaygrapltokens, reporting the output of the lexical parsing, which can help to narrow down bugs in your DSL input.

max-little commented 2 years ago

OK @fAndreuzzi , more detailed description of tutorials now included in the README, as requested.

fandreuz commented 2 years ago

Hey @max-little,

To explain in more detail, in your test the string 'hello' on its own doesn't have an end of command (EOC) terminator. In fact, 'hello;' (with the semicolon command terminator) would be a valid GRAPL DSL string, as you can check by running [...]

Thank you very much for the explanation, now it's clearer to me.

In any case, what you will now get, if you don't terminate a command, is the following:

grapl_obj.readgrapl('hello')
Grapl syntax error: unexpected end of input

Perfect! Thanks for the change.

I've just opened max-little/GRAPL#12 for a minor improvement in the readability of the README file (just to let the user quickly recognize where are the tutorials, how to run them and what's the content). Feel free to reject or merge depending on what you think about it.

My review is over I think, all my checkboxes are ticked @Nikoleta-v3

max-little commented 2 years ago

Excellent, thank you @fAndreuzzi for all your help and advice with this. All the best Max

max-little commented 2 years ago

Hi @Athene-ai (cc @Nikoleta-v3 )

Just a quick follow-up to ask if there were any additional issues you wanted to raise, or are you happy with the now revised codebase?

Best Max

Athene-ai commented 2 years ago

Hi @Athene-ai (cc @Nikoleta-v3 )

Just a quick follow-up to ask if there were any additional issues you wanted to raise, or are you happy with the now revised codebase?

Best Max

I am happy with the now revised codebase

max-little commented 2 years ago

Many thanks for the quick response @Athene-ai , that's great and thanks for all your advice!

@Nikoleta-v3 : I assume you are OK to proceed with the next step in the publication process, i.e. for us to make a tagged release of the software?

Best Max

Nikoleta-v3 commented 2 years ago

@fAndreuzzi and @Athene-ai thank you both for your time and your reviews! I believe that the package has improved a lot thanks to your comments πŸ™πŸ»

Nikoleta-v3 commented 2 years ago

@max-little what do you think about this:

I see that your code is not on PyPI, do you plan to upload it there such that your users can install GRAPL via pip install grapl?

Athene-ai commented 2 years ago

@fAndreuzzi and @Athene-ai thank you both for your time and your reviews! I believe that the package has improved a lot thanks to your comments πŸ™πŸ»

The pleasure is mine!

Nikoleta-v3 commented 2 years ago

@max-little once you have addressed my above, could you:

I can then move forward with accepting the submission. Let me know if you need any help with any of the above πŸ˜„

max-little commented 2 years ago

Thanks @Nikoleta-v3 , tagged release GRAPL v1.4 here: https://github.com/max-little/GRAPL/releases/tag/v1%2C4

Zenodo DOI of archived version: https://doi.org/10.5281/zenodo.6959433

max-little commented 2 years ago

@Nikoleta-v3 OK there's now also the option to install through PyPI: https://pypi.org/project/grapl-causal/1.4/

Nikoleta-v3 commented 2 years ago

Thank you!

Nikoleta-v3 commented 2 years ago

@editorialbot generate pdf

Nikoleta-v3 commented 2 years ago

@editorialbot check references

editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

editorialbot commented 2 years ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1017/S0266466603004109 is OK
- 10.1145/3501714.3501743 is OK
- 10.3389/fnagi.2020.553635 is OK

MISSING DOIs

- None

INVALID DOIs

- None
Nikoleta-v3 commented 2 years ago

@editorialbot set 10.5281/zenodo.6959433 as archive

editorialbot commented 2 years ago

Done! Archive is now 10.5281/zenodo.6959433

editorialbot commented 2 years ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1017/S0266466603004109 is OK
- 10.1145/3501714.3501743 is OK
- 10.3389/fnagi.2020.553635 is OK

MISSING DOIs

- None

INVALID DOIs

- None
Nikoleta-v3 commented 2 years ago

@editorialbot set v.1.4.0 as version

editorialbot commented 2 years ago

Done! version is now v.1.4.0

Nikoleta-v3 commented 2 years ago

@editorialbot set v1.4.0 as version

editorialbot commented 2 years ago

Done! version is now v1.4.0

Nikoleta-v3 commented 2 years ago

@editorialbot recommend-accept