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 obsolete functions from handy_functions #111

Merged
merged 6 commits into from
Feb 17, 2025

Conversation

pm-blanco
Copy link
Collaborator

@pm-blanco pm-blanco commented Feb 10, 2025

Removed

  • handy_functions.create_random_seed no longer needed because now instances of pyMBE take the random seed as input
  • handy_functions.visualize_espresso_system because it was not used anywhere in the library
  • handy_functions.do_snapshot_espresso_system moved to the tutorial because it was a function specific for it

@pm-blanco pm-blanco self-assigned this Feb 10, 2025
@pm-blanco pm-blanco added this to the pyMBE 1.0.0 milestone Feb 10, 2025
Comment on lines 31 to 46
"execution_count": null,
"execution_count": 1,
"metadata": {},
"outputs": [],
"outputs": [
{
"name": "stdout",
"output_type": "stream",
"text": [
"Current set of reduced units:\n",
"0.35 nanometer = 1 reduced_length\n",
"4.1164e-21 joule = 1 reduced_energy\n",
"1.6022e-19 coulomb = 1 reduced_charge\n",
"Temperature: 298.15 kelvin\n",
"The side of the simulation box is 7.5 nanometer = 21.428571428571427 reduced_length\n"
]
}
],
Copy link
Member

Choose a reason for hiding this comment

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

don't forget to clear output in the tutorial

Copy link
Contributor

Choose a reason for hiding this comment

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

Related to this: currently there are separate files for the tutorial and solution, so there is always the possibility of introducing incompatibilities between these two files. Is there some way to avoid this, e.g. a single file with initially hidden solutions as we do it for the ESPResSo tutorials?

Copy link
Collaborator Author

@pm-blanco pm-blanco Feb 16, 2025

Choose a reason for hiding this comment

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

@davidbbeyer what incompatibilities are you specifically referring to? Do you mean inconsistencies between naming of functions and variables or some other more serious issue?

I do not completely oppose to the idea of merging the solutions into the tutorial, although it does come with some drawbacks:

  • The feature with the hidden solutions is an extension for jupyter notebooks. I am not sure if we can enforce that such feature is active when using the pyMBE kernel @jngrad? Would that potentially create some issues for our users?
  • My experience when I see people doing the ESPResSo tutorials is that having the solution "one click away" becomes too tempting and they tend to simply read it before trying to solve the exercise themselves. Then again, having the solution "one file away" is not really preventing that much more...

Copy link
Contributor

Choose a reason for hiding this comment

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

What is mean is that basically the content (e.g. function names) could diverge between the two files, since we are not explicitly comparing them or deriving them from a single file.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @davidbbeyer, maintaining two files will inevitably lead to divergence. ESPResSo relies on a convoluted nbformat script to hide solution cells into collapsible cells. Developers can modify the notebooks directly, and CI pipelines can run them directly, while users need to run make tutorials in the build folder to get the processed notebooks with collapsed solutions cells.

There is no build folder in pyMBE, so it's a bit tricky. JupyterLab doesn't natively support the feature we need. The closest thing they offer is the %load command. Here is a MWE Jupyter notebook:

# In[ ]:
import numpy as np
import matplotlib.pyplot as plt

# In[ ]:
# %load solution1

# In[ ]:
plt.plot(np.arange(10), analytical(np.arange(10)), label="analytical")
plt.plot([1,2], [2,4], "o", label="simulation")
plt.legend()
plt.show()

Note how the second cell has a commented out %load command. Users can reveal the solution by uncommenting the line and executing the cell, which will paste the contents of solution1.py into the cell:

def analytical(xdata):
    return 2 * np.array(xdata)

The downside is that you need to manually clean up each %load cell individually, and you can't press the Run all button, because when the %load command is executed, it copy-pastes the code without executing it (you need to execute the cell twice). This makes maintenance quite annoying.

The exercise and exercise2 plug-ins from Jupyter Classic haven't been ported to JupyterLab (migration table). JupyterBook (python package jupyter-book) has collapsible code examples, but then you have to decide whether you want the hidden cells to automatically execute with Run all (nice for maintainers, very annoying for users since their code is executed after the solution, which can lead to particles being added twice to the system) or not (annoying for maintainers). It doesn't seem the behavior can be easily changed for the entire document. It's also unclear to me if this extension works inside other Jupyter IDEs, like VS Code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do not see any clear solution to this issue. Of course, it would be better to avoid inconsistencies between the tutorial and the solution but I am not convinced by the alternatives either. I think that this discussion is out of the scope of this PR, so I propose that we discuss it in our next monthly meeting and we move forward with this PR unless there is something else to be fixed here.

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed

Copy link
Contributor

@davidbbeyer davidbbeyer left a comment

Choose a reason for hiding this comment

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

I agree with the comments raised by @jngrad. Another thing I noticed when looking at handy_functions.py again: the print() statement in line 64 seems superfluous. Could you check and remove it?

Comment on lines 31 to 46
"execution_count": null,
"execution_count": 1,
"metadata": {},
"outputs": [],
"outputs": [
{
"name": "stdout",
"output_type": "stream",
"text": [
"Current set of reduced units:\n",
"0.35 nanometer = 1 reduced_length\n",
"4.1164e-21 joule = 1 reduced_energy\n",
"1.6022e-19 coulomb = 1 reduced_charge\n",
"Temperature: 298.15 kelvin\n",
"The side of the simulation box is 7.5 nanometer = 21.428571428571427 reduced_length\n"
]
}
],
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to this: currently there are separate files for the tutorial and solution, so there is always the possibility of introducing incompatibilities between these two files. Is there some way to avoid this, e.g. a single file with initially hidden solutions as we do it for the ESPResSo tutorials?

pm-blanco and others added 4 commits February 16, 2025 11:06
Co-authored-by: Jean-Noël Grad <jgrad@icp.uni-stuttgart.de>
Co-authored-by: Jean-Noël Grad <jgrad@icp.uni-stuttgart.de>
@pm-blanco pm-blanco merged commit fb39c69 into pyMBE-dev:main Feb 17, 2025
3 checks passed
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.

3 participants