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

Script interface maintenance #4482

Merged
merged 12 commits into from
Apr 4, 2022
Merged

Script interface maintenance #4482

merged 12 commits into from
Apr 4, 2022

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented Mar 29, 2022 •
edited
Loading

Description of changes:

  • API change: rename espressomd.reaction_ensemble to espressomd.reaction_methods
  • feature configuration: remove obsolete EXPERIMENTAL_FEATURES
  • script interface:
    • use shared pointers instead of new expressions in the script interface
    • make type conversion error messages less ambiguous by putting quotes around C++ types
    • require EK node grid indices to be exactly 3 integers (2 indices would skip the exception and return None)
  • python interface:
    • replace from X import Y by import X and X.Y in samples and python interface
    • use tqdm, numpy and f-strings more often in the samples
    • remove lj-demo.py (fixes lj-demo.py is broken #4347)
    • remove mentions to non-existent functions in the user guide

@jngrad
script_interface: Use shared pointers
e23f332
@jngrad
docs: Remove outdated information
8970a69
@jngrad
python: Modernize samples
f1370ec 
Use tqdm instead of print statements. Use f-strings and numpy tools
more frequently. Avoid using `from X import Y` (namespace pollution).
@jngrad
python: Cleanup import statements
8d5342b 
Replace `from X import Y` by `import X` and `X.Y` in the Cython
interface and remove unused imports.
@jngrad
python: Rename reaction_ensemble to reaction_methods
1111580
@jngrad
python: Add quotes around Python/C++ types
ef66c79
@jngrad
python: Make benchmarks deterministic
9671afc
@jngrad jngrad added this to the Espresso 4.2 milestone Mar 29, 2022
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@jngrad
docs: Cleanup documentation
290ed98 
Fix typos and line breaks. Remove duplicated variable types (the
System class property docstrings contain the up-to-date types).
@jngrad
samples: Remove lj-demo.py
8be90ab 
The physics is broken since release 4.0.0: logarithm input values
can be null or negative when moving the GUI sliders and the phase
diagram is incorrect. The script is also completely untested and
the NpT integrator is not properly configured.
@jngrad jngrad force-pushed the script_interface_maintenance branch from ed3e388 to 8be90ab Compare March 29, 2022 00:57
@jngrad jngrad marked this pull request as ready for review March 30, 2022 15:38
christophlohrmann
christophlohrmann previously approved these changes Apr 1, 2022
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.

PR seems very fine to me. One point though:
As far as I know, we need lj-demo.py for Tag der Wissenschaft. Is there anyone actively working to fix the demo?

src/python/espressomd/analyze.pyx
@@ -560,7 +559,7 @@ class Analysis:
raise ValueError("type_list_b has to be a list!")

if r_max is None:
box_l = make_array_locked(< Vector3d > box_geo.length())
Copy link
Contributor

Choose a reason for hiding this comment

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

How was this never caught by anyone?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure, there should have been a warning about the redundant cast according to the Cython docs on type casting.

src/python/espressomd/electrokinetics.pyx Outdated Show resolved Hide resolved
@jngrad
Copy link
Member Author

jngrad commented Apr 1, 2022

As far as I know, we need lj-demo.py for Tag der Wissenschaft. Is there anyone actively working to fix the demo?

Not that I know. I sent an email to David and Julian on March 22 to inform them of the planned removal and invite them to voice their objection, but got no reply. For the TdW event, people can use PyStar (#4382). When I used lj-demo.py for TdW 2019, people asked why the LJ simulation was following P=VNRT instead of PV=NRT, and I had no answer. The sample is broken since release 4.0.0, and unfortunately no one here was successful in implementing a working version. It is time to retire it.

@jngrad
python: Require three integers for EK node indices
d9964ec 
Also use more specific exceptions.
@jngrad jngrad requested a review from reinaual April 1, 2022 16:52
@jngrad jngrad added the automerge Merge with kodiak label Apr 4, 2022
@kodiakhq kodiakhq bot merged commit 294dcab into espressomd:python Apr 4, 2022
@jngrad jngrad deleted the script_interface_maintenance branch April 4, 2022 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reinaual reinaual reinaual approved these changes

@christophlohrmann christophlohrmann christophlohrmann left review comments

Assignees
No one assigned
Labels
ApiChange automerge Merge with kodiak Documentation Python ScriptInterface
Projects
None yet
Milestone
Espresso 4.2.0
Development

Successfully merging this pull request may close these issues.

lj-demo.py is broken
3 participants
@jngrad @reinaual @christophlohrmann