Modify to get # of Lanczos iterations and set tolerance #6
Conversation
| @@ -17,6 +17,7 @@ namespace uammd{ | |||
| real viscosity; | |||
| real hydrodynamicRadius = -1; //If not provided it will be taken from pd->getRadius if possible | |||
| real tolerance = 1e-3; | |||
| real LanczosTol; | |||
There was a problem hiding this comment.
Call this lanczosTolerance to be consistent with the naming
There was a problem hiding this comment.
Also, give it a default value that signals "not set":
real lanczosTolerance = -1;
| int getNumLanczosIterations(){ | ||
| return nearField-> getNumLanczosIterations(); | ||
| } | ||
|
|
There was a problem hiding this comment.
This are the only lines that should be added to this file in your PR
There was a problem hiding this comment.
This file should present no changes. You mixed the sheared functionality and the lanczos iterations thing.
| int getNumLanczosIterations(){ | ||
| return lanczos->getLastRunRequiredSteps(); | ||
| } |
There was a problem hiding this comment.
These lines are the only ones that should be changed in this file
| if(not lanczos){ | ||
| //It appears that this tolerance is unnecesary for lanczos, but I am not sure so better leave it like this. | ||
| this->lanczosTolerance = this->tolerance; //std::min(0.05f, sqrt(par.tolerance)); | ||
| //this->lanczosTolerance = this->tolerance; //std::min(0.05f, sqrt(par.tolerance)); |
There was a problem hiding this comment.
This line should be removed.
| tolerance(par.tolerance) | ||
| { |
There was a problem hiding this comment.
You are not setting lanczosTolerance here, meaning that the parameter that you added in BDHI is completely ignored here. Furthermore, you did not give this parameter a default value, which means any code using PSE currently would be broken.
Please add a default value for the parameter here, as:
tolerance(par.tolerance),
lanczosTolerance(par.lanczosTolerance){
if(this->lanczosTolerance<0) this->lanczosTolerance = std::min(0.05f, sqrt(par.tolerance));This way the old behavior is mantained if a user does not know about your new parameter.
| @@ -216,7 +264,7 @@ namespace uammd{ | |||
| thrust::transform(thrust::cuda::par.on(st), id_tr, id_tr + numberParticles, noise.begin(), | |||
| pse_ns::SaruTransform(noise_prefactor, seed, seed2)); | |||
| lanczos->run(Mvdot_near, (real*) BdW, (real*)noise.data().get(), | |||
| tolerance, 3*numberParticles, st); | |||
There was a problem hiding this comment.
Your PR should not modify this file
There was a problem hiding this comment.
Hi @omaxian, thank you so much for the effort!
You included a lot of changes here, as you say you probably copied files from the incorrect place.
You introduced the sheared functionality in this PR, which is already in another branch. The one you added here is not covered in the tests and in fact contains several bugs at first glance (the temperature is ignored in some places, and the lanczosTolerance is also ignored). Thus I cannot merge this as it is.
Would you please consider reviewing my comments?
I don't know why there are also changes for the shear here. I think I am not comparing to the correct version on your end. You can see the Lanczos changes are quite simple.