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

improve documentation of the Widom insertion method #3254

Merged

Conversation

jonaslandsgesell
Copy link
Member

@jonaslandsgesell jonaslandsgesell commented Oct 16, 2019

This PR adds documentation for the Widom insertion method.

In addition it adds the feature to change reaction constants and delete reactions. This is functionality which Oleg (@helvrud) suggested and which gives some more flexibility to the user.

@codecov
Copy link

codecov bot commented Oct 16, 2019

Codecov Report

Merging #3254 into python will decrease coverage by <1%.
The diff coverage is 75%.

Impacted file tree graph

@@          Coverage Diff           @@
##           python   #3254   +/-   ##
======================================
- Coverage      86%     86%   -1%     
======================================
  Files         538     538           
  Lines       25346   25350    +4     
======================================
- Hits        21831   21830    -1     
- Misses       3515    3520    +5
Impacted Files Coverage Δ
src/core/reaction_ensemble.hpp 78% <100%> (ø) ⬆️
src/core/reaction_ensemble.cpp 86% <50%> (-1%) ⬇️
src/core/particle_data.cpp 96% <0%> (-1%) ⬇️
src/core/electrostatics_magnetostatics/p3m.cpp 85% <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 0c813ca...67a9ef5. Read the comment docs.

@RudolfWeeber
Copy link
Contributor

@kosovan, would you be willing to review this?

@kosovan
Copy link
Contributor

kosovan commented Oct 18, 2019

@kosovan, would you be willing to review this?

@RudolfWeeber I should be able to do it on Tuesday or Wednesday. I am not sure if that is not too late but I have some teaching duties that cannot be postponed.

… set. For generality I also added the ability to remove reactions which were added to a reaction system
@jonaslandsgesell
Copy link
Member Author

@helvrud I implemented the ability to change the reaction constant or delete reactions you already added:

deletion of a given reaction:

RE.delete_reaction(1)

change of the reaction constant (back and forward reaction simultaneously):

reaction_id=0
RE.change_reaction_constant(reaction_id, new_reaction_constant)

@RudolfWeeber RudolfWeeber self-assigned this Oct 21, 2019
doc/sphinx/advanced_methods.rst Outdated Show resolved Hide resolved
doc/sphinx/advanced_methods.rst Outdated Show resolved Hide resolved
doc/sphinx/advanced_methods.rst Outdated Show resolved Hide resolved
doc/sphinx/advanced_methods.rst Outdated Show resolved Hide resolved
doc/sphinx/advanced_methods.rst Outdated Show resolved Hide resolved
doc/sphinx/advanced_methods.rst Show resolved Hide resolved
src/python/espressomd/reaction_ensemble.pyx Show resolved Hide resolved
src/python/espressomd/reaction_ensemble.pyx Outdated Show resolved Hide resolved
src/python/espressomd/reaction_ensemble.pyx Show resolved Hide resolved
doc/sphinx/advanced_methods.rst Outdated Show resolved Hide resolved
@RudolfWeeber
Copy link
Contributor

@jonaslandsgesell this pr adds functionallity in addition to docs. Please edit the description to explain this PRś intention

@RudolfWeeber
Copy link
Contributor

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 19, 2019

👎 Rejected by code reviews

@jonaslandsgesell
Copy link
Member Author

@kosovan could you mark your comments as resolved? Otherwise the merge cannot proceed and people do not find documentation about the method. If there are remaining problems please open a new issue for them.

@KaiSzuttor
Copy link
Member

@jonaslandsgesell that's an issue of bors actually

@KaiSzuttor
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 20, 2019

👎 Rejected by code reviews

@jngrad
Copy link
Member

jngrad commented Nov 20, 2019

@KaiSzuttor it's actually a feature, bors won't merge if anyone requested a change, until that person approves the PR. You can dismiss kosovan's review if you want to proceed with bors.

@jngrad
Copy link
Member

jngrad commented Nov 20, 2019

Should this PR be milestoned as 4.2 or 4.1.2?

@KaiSzuttor
Copy link
Member

@jngrad personally I think only github should decide whether the PR is ready to merge. Also, it seems that I'm not able to dismiss the review of @kosovan. So we can only manually merge.

@KaiSzuttor
Copy link
Member

image

... and the API says it's ready to merge

@RudolfWeeber
Copy link
Contributor

RudolfWeeber commented Nov 20, 2019 via email

@KaiSzuttor KaiSzuttor merged commit f23a4d0 into espressomd:python Nov 20, 2019
@kosovan
Copy link
Contributor

kosovan commented Nov 20, 2019

Hey guys,

I apologize for the late response. I was not aware that this was waiting for me to mark my requests as resolved. Sorry if I delayed the merge unnecessarily.

@KaiSzuttor
Copy link
Member

@kosovan as I said, that's not your fault. It's a bug of the mergebot.

@jngrad jngrad added this to the Espresso 4.1.2 milestone Nov 22, 2019
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.

5 participants