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

Remove end keyword from NESTML models #866

Merged
merged 40 commits into from
Apr 5, 2023
Merged

Conversation

pnbabu
Copy link
Contributor

@pnbabu pnbabu commented Feb 16, 2023

Fixes #586

@jougs jougs requested a review from diesmann February 23, 2023 13:54
@pnbabu pnbabu mentioned this pull request Mar 6, 2023
@pnbabu pnbabu marked this pull request as ready for review March 9, 2023 09:22
@pnbabu pnbabu requested a review from clinssen March 9, 2023 09:23
Copy link
Contributor

@clinssen clinssen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this heroic effort! I just have a few minor comments.

doc/models_library/aeif_cond_alpha.rst Outdated Show resolved Hide resolved
doc/models_library/aeif_cond_alpha.rst Outdated Show resolved Hide resolved
doc/tutorials/izhikevich/nestml_izhikevich_tutorial.ipynb Outdated Show resolved Hide resolved
pynestml/grammars/PyNestMLParser.g4 Show resolved Hide resolved
pynestml/grammars/generate_lexer_parser Outdated Show resolved Hide resolved
tests/comment_test.py Outdated Show resolved Hide resolved
tests/symbol_table_builder_test.py Outdated Show resolved Hide resolved
tests/test_model_without_end_keyword.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many, many thanks! This looks like a massive improvement! I have added some comments to the code and have one general question about the documentation: Does it really make sense to have all the .rst files in the repository? Can't they just be generated on Read the Docs and locally when they are needed?

doc/models_library/hh_cond_exp_destexhe.rst Outdated Show resolved Hide resolved
doc/nestml_language/nestml_language_concepts.rst Outdated Show resolved Hide resolved
doc/nestml_language/nestml_language_concepts.rst Outdated Show resolved Hide resolved
doc/nestml_language/neurons_in_nestml.rst Show resolved Hide resolved
@pnbabu
Copy link
Contributor Author

pnbabu commented Mar 17, 2023

Many, many thanks! This looks like a massive improvement! I have added some comments to the code and have one general question about the documentation: Does it really make sense to have all the .rst files in the repository? Can't they just be generated on Read the Docs and locally when they are needed?

@jougs I have created an issue to separate building the models documentation #880. We will address it in a separate PR.

@pnbabu pnbabu requested review from jougs and clinssen March 17, 2023 16:18
Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions and a question on formulations of the text. Otherwise, I think I'm fine.

doc/nestml_language/nestml_language_concepts.rst Outdated Show resolved Hide resolved
doc/nestml_language/nestml_language_concepts.rst Outdated Show resolved Hide resolved
doc/nestml_language/nestml_language_concepts.rst Outdated Show resolved Hide resolved
@pnbabu
Copy link
Contributor Author

pnbabu commented Mar 22, 2023

Generated docs: https://nestml-doc.readthedocs.io/en/latest/

Copy link
Contributor

@clinssen clinssen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the fantastic work!

Copy link

@diesmann diesmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice work. I checked the concept and the documentation.

@pnbabu pnbabu merged commit 7a1d0ce into nest:master Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove end keyword
4 participants