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.

Sign up for GitHub

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

Jump to bottom

Electrodes/constant potential tutorial #4784

Merged
merged 3 commits into from
Oct 2, 2023
Merged

Electrodes/constant potential tutorial #4784

merged 3 commits into from
Oct 2, 2023

Conversation

schlaicha
Copy link
Contributor

@schlaicha schlaicha commented Sep 5, 2023 •
edited by jngrad
Loading

Fixes #3955

Description of changes:

  • Write new tutorial on ICC/ELC/ELC-IC

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@schlaicha schlaicha mentioned this pull request Sep 5, 2023
Closed
@schlaicha schlaicha marked this pull request as draft September 6, 2023 06:52
@schlaicha schlaicha changed the title WIP: new tutorial on electrodes/constant potential new tutorial on electrodes/constant potential Sep 18, 2023
@schlaicha schlaicha marked this pull request as ready for review September 18, 2023 16:16
@schlaicha
Copy link
Contributor Author

The tutorial with the test parameters runs in about 3 minutes on my M1 Macbook, no clue why the CI takes 20 minutes...
Otherwise this should be ready for review @jngrad @reinaual
I will fix the tests after in-person discussion on the CI performance, we can also take out some tests since now all physics is checked more than once...

christophlohrmann
christophlohrmann suggested changes Sep 26, 2023
View reviewed changes
Copy link
Contributor

@christophlohrmann christophlohrmann left a comment

Choose a reason for hiding this comment

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

part 1: overall well written, but the task needs a bit of work to become doable/didactically meaningful for users

doc/tutorials/electrodes/electrodes_part1.ipynb Outdated Show resolved Hide resolved
doc/tutorials/electrodes/electrodes_part1.ipynb Outdated Show resolved Hide resolved
doc/tutorials/electrodes/electrodes_part1.ipynb Outdated Show resolved Hide resolved
doc/tutorials/electrodes/electrodes_part1.ipynb Outdated Show resolved Hide resolved
doc/tutorials/electrodes/electrodes_part1.ipynb Outdated Show resolved Hide resolved
doc/tutorials/electrodes/electrodes_part1.ipynb Outdated Show resolved Hide resolved
doc/tutorials/electrodes/electrodes_part1.ipynb Outdated Show resolved Hide resolved
doc/tutorials/electrodes/electrodes_part1.ipynb Outdated Show resolved Hide resolved
doc/tutorials/electrodes/electrodes_part1.ipynb Outdated Show resolved Hide resolved
doc/tutorials/electrodes/electrodes_part1.ipynb Outdated Show resolved Hide resolved
christophlohrmann
christophlohrmann suggested changes Sep 26, 2023
View reviewed changes
Copy link
Contributor

@christophlohrmann christophlohrmann left a comment

Choose a reason for hiding this comment

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

part 2:

  • Needs to be read again for small grammar mistakes/typos
  • Tasks do not have the same style as in the other tutorials
  • The plots that result from the simulation need to be interpreted physically

doc/tutorials/electrodes/electrodes_part2.ipynb Show resolved Hide resolved
doc/tutorials/electrodes/electrodes_part2.ipynb Outdated Show resolved Hide resolved
doc/tutorials/electrodes/electrodes_part2.ipynb Outdated Show resolved Hide resolved
doc/tutorials/electrodes/electrodes_part2.ipynb Show resolved Hide resolved
doc/tutorials/electrodes/electrodes_part2.ipynb Outdated Show resolved Hide resolved
doc/tutorials/electrodes/electrodes_part2.ipynb Outdated Show resolved Hide resolved
@schlaicha
Copy link
Contributor Author

Thank you @christophlohrmann for reviewing and your changes, this helped a lot!

From my side this is perfectly ready now! @reinaual

christophlohrmann
christophlohrmann approved these changes Sep 27, 2023
View reviewed changes
Copy link
Contributor

@christophlohrmann christophlohrmann left a comment

Choose a reason for hiding this comment

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

approved I guess

@jngrad jngrad force-pushed the constant-potential_tutorial branch from 8e06340 to caf6617 Compare October 2, 2023 20:04
@jngrad jngrad changed the title new tutorial on electrodes/constant potential Electrodes/constant potential tutorial Oct 2, 2023
@jngrad
Rework electrode tutorials
58f088e 
Copy editing. Reduce code duplication. Improve matplotlib figures.
@jngrad jngrad force-pushed the constant-potential_tutorial branch from caf6617 to 58f088e Compare October 2, 2023 20:10
@jngrad jngrad added this to the ESPResSo 4.3.0 milestone Oct 2, 2023
@jngrad jngrad added Documentation automerge Merge with kodiak labels Oct 2, 2023
@kodiakhq kodiakhq bot merged commit a931825 into espressomd:python Oct 2, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reinaual reinaual reinaual left review comments

@jngrad jngrad jngrad approved these changes

@christophlohrmann christophlohrmann christophlohrmann approved these changes

Assignees
No one assigned
Labels
automerge Merge with kodiak Documentation
Projects
None yet
Milestone
ESPResSo 4.3.0
Development

Successfully merging this pull request may close these issues.

Write Tutorial 02 part 2: ELC system with dielectric boundaries
4 participants
@schlaicha @reinaual @jngrad @christophlohrmann