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

Refactor NpT public interface #3253

Merged
merged 7 commits into from
Oct 18, 2019
Merged

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented Oct 16, 2019

Description of changes:

  • remove the silent conversion of the incorrect input parameter dimension=[0,0,0] to [1,1,1] in the core (bypassing sanity checks), now the checks will throw an exception for fixed-volume NpT; the original behavior was counter-intuitive and undocumented until 2 days ago
  • remove the automatic decay of NpT to NVT upon initialization of NpT with incorrect parameters
  • remove unused p_inst_av variable (average instantaneous pressure)
  • cleanup integrator documentation

Allow bool values in the script interface. The default value for
box dimension fluctuations is now [True, True, True] instead of
the counter-intuitive [0, 0, 0] that was implicitly converted to
[1, 1, 1] in the core.
Average instantaneous pressure: instantaenous pressure divided by
the number of steps in the integration (NaN if nsteps=0). Unused in
the core and observables, inaccessible from the Python interface.
When the NpT integrator is initialized with incorrect values, halt
script execution with an exception instead of initializing NVT.
@jngrad jngrad added the BugFix label Oct 16, 2019
@jngrad jngrad requested a review from RudolfWeeber October 16, 2019 09:14
@codecov
Copy link

codecov bot commented Oct 16, 2019

Codecov Report

Merging #3253 into python will increase coverage by <1%.
The diff coverage is 95%.

Impacted file tree graph

@@           Coverage Diff           @@
##           python   #3253    +/-   ##
=======================================
+ Coverage      85%     85%   +<1%     
=======================================
  Files         531     531            
  Lines       25796   25766    -30     
=======================================
- Hits        22169   22154    -15     
+ Misses       3627    3612    -15
Impacted Files Coverage Δ
src/core/npt.cpp 92% <ø> (-2%) ⬇️
src/core/global.cpp 83% <ø> (ø) ⬆️
src/core/pressure.cpp 90% <ø> (ø) ⬆️
src/core/electrostatics_magnetostatics/coulomb.cpp 79% <ø> (ø) ⬆️
src/core/communication.cpp 94% <100%> (ø) ⬆️
src/core/integrators/velocity_verlet_npt.cpp 85% <100%> (+1%) ⬆️
src/core/integrate.cpp 81% <94%> (+6%) ⬆️
src/core/electrostatics_magnetostatics/p3m.cpp 86% <0%> (-1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34c6bd7...98e2749. Read the comment docs.

@fweik fweik self-assigned this Oct 16, 2019
@fweik fweik removed their assignment Oct 16, 2019
@jngrad jngrad added this to the Espresso 4.1.1 milestone Oct 16, 2019
@jngrad
Copy link
Member Author

jngrad commented Oct 18, 2019

bors r=fweik

bors bot added a commit that referenced this pull request Oct 18, 2019
3216: Refactor ghosts.cpp r=KaiSzuttor a=hirschsn

More refactoring that builds upon #3212 

Description of *major* changes:
 - remove GhostCommunication::mpi_comm as it is not used in ghosts.cpp,
 - make GhostCommunication::part_lists a std::vector,
 - remove static variables s_buffer and r_buffer,
 - factor out memory handling,
 - change loops to range based for,
 - use boost::mpi.
 - Replace the manual poststore and prefetch loops by find_ifs

Mainly, ghosts.cpp now defines CommBuf, which is a container for the data to be sent or received as well as two classes (Archiver and BondArchiver), that insert and extract the memory from CommBuf.

Some of these changes, like the removal of static variables, is necessary for my implementation of asynchronous ghost communication.

PR Checklist
------------
 - [ ] Tests?
   - [ ] Interface
   - [ ] Core 
 - [ ] Docs?


3239: Added test criteria for the charged_system-2 tutorial r=RudolfWeeber,jngrad a=reinaual



3253: Refactor NpT public interface r=fweik a=jngrad

Description of changes:

- remove the silent conversion of the incorrect input parameter `dimension=[0,0,0]` to `[1,1,1]` in the core (bypassing sanity checks), now the checks will throw an exception for fixed-volume NpT; the original behavior was counter-intuitive and undocumented until 2 days ago
- remove the automatic decay of NpT to NVT upon initialization of NpT with incorrect parameters
- remove unused `p_inst_av` variable (average instantaneous pressure)
- cleanup integrator documentation


3258: CMake minor fixes r=fweik a=jngrad

Description of changes:
- change next milestone to 4.2
- load `GNUInstallDirs` to make standard GNU paths accessible from CMake variables
- simplify CMake logic and install in `python3.X` folder instead of the deprecated `python3` folder
- add extra check to make sure install paths are correctly configured (all python and shared object files must be inside the package `espressomd`)


Co-authored-by: Steffen Hirschmann <[email protected]>
Co-authored-by: Florian Weik <[email protected]>
Co-authored-by: Alexander Reinauer <[email protected]>
Co-authored-by: Jean-Noël Grad <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 18, 2019

Build succeeded

@bors bors bot merged commit 98e2749 into espressomd:python Oct 18, 2019
@jngrad jngrad deleted the npt-interface-refactor branch January 18, 2022 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants