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

Bestpractice #464

Closed
wants to merge 3 commits into from
Closed

Conversation

mkhalil8
Copy link
Contributor

I have made two files with the same changes, kindly ignore the second one.

@ntolley
Copy link
Contributor

ntolley commented Feb 16, 2022

@mkhalil8 thanks for starting the pull request! I'm a little confused by the file that's committed here, is it the output after building the html?

In any case a good example of editing existing documentation is #249. You can see that the "diff" when you click on "files changed" only includes desired updates in green, while a large amount remains untouched.

There's unfortunately a bit of a learning curve to merging PR's, but all the others are infinitely easier after the first one. Please let us if you want to set up a meeting about contributing, and the subject of this PR in particular!

Comment on lines +417 to +420
.. warning::
Always look at dipoles in conjunction with raster plots and spike histogram to avoid misinterpretation.

Run multiple trials for your simulation to get an average of different drives seeds before drawing conclusions.
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should put this in the python script as comments. This file you edited is auto generated ...

Copy link
Contributor Author

@mkhalil8 mkhalil8 Feb 17, 2022

Choose a reason for hiding this comment

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

You should put this in the python script as comments. This file you edited is auto generated ...

@jasmainak 👍 I thought adjusting the .rst file is how to do it :)
will update the python script file then make a new pull request.

Copy link
Collaborator

Choose a reason for hiding this comment

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

just push to the same branch. You don't need to make a new pull request ...

@mkhalil8
Copy link
Contributor Author

mkhalil8 commented Feb 17, 2022

@mkhalil8 thanks for starting the pull request! I'm a little confused by the file that's committed here, is it the output after building the html?

In any case a good example of editing existing documentation is #249. You can see that the "diff" when you click on "files changed" only includes desired updates in green, while a large amount remains untouched.

There's unfortunately a bit of a learning curve to merging PR's, but all the others are infinitely easier after the first one. Please let us if you want to set up a meeting about contributing, and the subject of this PR in particular!

@ntolley yes that would be great if we can have a meeting, I believe it will not be my only time to try contributing, so help is needed :)
I have read a bit about Sphinx and my understanding was that the .rst files are the ones that cause changes to the html. I updated the plot_simulate_evoked.rst and the html page followed so I thought I am on the right track :)

@jasmainak
Copy link
Collaborator

jasmainak commented Feb 17, 2022

would be great if you and @ntolley can meet. But ultimately you'll learn from the mistakes :)

sphinx-gallery is what we use: https://sphinx-gallery.github.io/stable/index.html. It's a layer on top of sphinx that converts the python files into rst that is finally built into html. The automatically generated rst should not be touched. You can check the rendering by doing:

$ cd doc/
$ make html-noplot

or $ make html if you want to run the examples and see the outputs as well. The htmls are stored in _build/html

@jasmainak
Copy link
Collaborator

@mkhalil8 any luck here?

@mkhalil8
Copy link
Contributor Author

@jasmainak I had couple of trials but something is off and the HTML is not updated. I have a meeting with Nick on Friday morning to sort it out.

@mkhalil8 mkhalil8 closed this Feb 25, 2022
@mkhalil8 mkhalil8 deleted the bestpractice branch February 25, 2022 14:34
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.

3 participants