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

testsuite: add test for is_valid_type. #3080

Merged
merged 8 commits into from
Aug 22, 2019
Merged

Conversation

KaiSzuttor
Copy link
Member

Currently it is possible to give nan as values to the particle properties because in python a nan is of type float. This PR adds a check for None, inf and nan to the is_valid_type and tests for the correct behavior.

testsuite/python/utils.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 13, 2019

Codecov Report

Merging #3080 into python will decrease coverage by <1%.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           python   #3080   +/-   ##
======================================
- Coverage      83%     83%   -1%     
======================================
  Files         530     530           
  Lines       26100   26083   -17     
======================================
- Hits        21915   21899   -16     
+ Misses       4185    4184    -1
Impacted Files Coverage Δ
src/core/thermostat.cpp 97% <0%> (-2%) ⬇️
src/core/constraints/ShapeBasedConstraint.cpp 87% <0%> (-1%) ⬇️
src/core/forces_inline.hpp 82% <0%> (-1%) ⬇️
src/core/thermostat.hpp 97% <0%> (-1%) ⬇️
src/core/electrostatics_magnetostatics/p3m.cpp 85% <0%> (-1%) ⬇️
src/core/pressure.cpp 90% <0%> (-1%) ⬇️
src/core/particle_data.cpp 95% <0%> (-1%) ⬇️
src/core/grid.hpp 100% <0%> (ø) ⬆️
...re/bonded_interactions/bonded_interaction_data.hpp 100% <0%> (ø) ⬆️
src/core/forces.cpp 100% <0%> (ø) ⬆️
... and 2 more

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 2948859...d85e112. Read the comment docs.

@fweik
Copy link
Contributor

fweik commented Aug 13, 2019

is_valid_type is a horrible name for the function. It checks values, and not types, also it returns false for most valid types. Regarding the value, inf is a valid value that should be settable.

@KaiSzuttor
Copy link
Member Author

Regarding the value, inf is a valid value that should be settable

why?

@fweik
Copy link
Contributor

fweik commented Aug 13, 2019

Because you don't know if inf is a valid value for all properties of all methods present and future.
In many cases it even behaves as expected out of the box. E.g. you could set mass to infinity, which
would fix the particle. gamma infinity could mean tracer coupling, ... Also I don't see a downside. Why do you want to exclude that?

@@ -151,6 +151,8 @@ cdef class ParticleHandle:

def __set__(self, _pos):
cdef double mypos[3]
if np.isnan(pos).any() or np.isinf(pos).any():
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you want to check that? I'm not sure that I follow. What scenario is this check for?

Copy link
Member Author

Choose a reason for hiding this comment

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

it seems that the callbacks do not handle nan. Please try:

import espressomd

system = espressomd.System(box_l=[10.0]*3)
system.part.add(pos=[float('nan')]*3)     

Copy link
Member Author

Choose a reason for hiding this comment

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

since pos is the only particle property where the target node depends on the value of the property we should validate before calling the callback.

@jngrad jngrad mentioned this pull request Aug 14, 2019
@KaiSzuttor
Copy link
Member Author

bors r=jngrad

bors bot added a commit that referenced this pull request Aug 22, 2019
3080: testsuite: add test for is_valid_type. r=jngrad a=KaiSzuttor

Currently it is possible to give ```nan``` as values to the particle properties because in python a ```nan``` is of type ```float```. This PR adds a check for ```None```, ```inf``` and ```nan``` to the ```is_valid_type``` and tests for the correct behavior.

Co-authored-by: Kai Szuttor <[email protected]>
Co-authored-by: Kai Szuttor <[email protected]>
@jngrad
Copy link
Member

jngrad commented Aug 22, 2019

bors r-
the webhook failed again

@bors
Copy link
Contributor

bors bot commented Aug 22, 2019

Canceled

@jngrad
Copy link
Member

jngrad commented Aug 22, 2019

bors r+

bors bot added a commit that referenced this pull request Aug 22, 2019
3070: Thermalized bond philox r=jngrad a=KonradBreitsprecher

Fixes the TB part of #2683 

- Common philox counter for all thermalized bonds. Decorrelation via the bond partner IDs (not taking into account the bond ID). This was much easier to implement and is valid, as two (different) thermalized bonds on the same particle pair makes no sense.

3080: testsuite: add test for is_valid_type. r=jngrad a=KaiSzuttor

Currently it is possible to give ```nan``` as values to the particle properties because in python a ```nan``` is of type ```float```. This PR adds a check for ```None```, ```inf``` and ```nan``` to the ```is_valid_type``` and tests for the correct behavior.

3096: Core: Correct maximum permissible skin in tune_skin() r=jngrad a=RudolfWeeber

Fixes #2924 
Fix according to the discussion in #2924 




Co-authored-by: KonradBreitsprecher <[email protected]>
Co-authored-by: Konrad Breitsprecher <[email protected]>
Co-authored-by: Konrad Breitsprecher <[email protected]>
Co-authored-by: Jean-Noël Grad <[email protected]>
Co-authored-by: Kai Szuttor <[email protected]>
Co-authored-by: Kai Szuttor <[email protected]>
Co-authored-by: Rudolf Weeber <[email protected]>
@bors
Copy link
Contributor

bors bot commented Aug 22, 2019

Build succeeded

@bors bors bot merged commit d85e112 into espressomd:python Aug 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.

3 participants