add threshold_lo_ & threshold_hi_ #816
Conversation
fix the issue "If the system has exploded, unphysical structures may be saved since no upper bound is set on the uncertainty threshold."
|
Thanks for the contribution. It seems you separated changes into two pull requests? Could you complete one and delete the other? |
|
Dear Prof. Fan, Thanks for your reply. One is for modifying active.cu another one is for modifying activate.cuh. I’m not sure how to delete another pull request. Could you merge the two in one pull request? Thanks Hao-Jen You |
|
OK, I will take care of it. I need to study the code more carefully when I have enough time. I will contact you if I have more questions. |
| @@ -238,7 +241,7 @@ void Active::process( | |||
| } | |||
| } | |||
| write_uncertainty(step, global_time, uncertainty); | |||
| if (uncertainty > threshold_) { | |||
| if (threshold_hi_ > uncertainty && uncertainty > threshold_lo_) { | |||
There was a problem hiding this comment.
OK, it is correct. read it wrongly.
|
@elindgren Do you think it is necessary to have an upper limit? |
There was a problem hiding this comment.
@Youhaojen I think it makes sense to have an upper cutoff for the use case that you describe. The high cutoff could potentially be an internal value, but it would probably be difficult to know what an appropriate value would be in general.
For readability, I would suggest renaming threshold_lo_ to threshold_high_ and threshold_hi_ to threshold_high_, respectively.
Also, modify the documentation for the active keyword to reflect the new usage, including the examples. The relevant file can be found here: https://github.com/brucefan1983/GPUMD/blob/master/doc/gpumd/input_parameters/active.rst
| " will check if uncertainties exceed %f and %f every %d iterations.\n", | ||
| threshold_lo_, threshold_hi_, |
There was a problem hiding this comment.
| " will check if uncertainties exceed %f and %f every %d iterations.\n", | |
| threshold_lo_, threshold_hi_, | |
| " will check if uncertainties exceed %f and are below %f every %d iterations.\n", | |
| threshold_lo_, threshold_hi_, |
| if (!is_valid_real(param[4], &threshold_lo_)) { | ||
| PRINT_INPUT_ERROR("threshold should be a real number.\n"); | ||
| } | ||
| if (!is_valid_real(param[5], &threshold_hi_)) { |
There was a problem hiding this comment.
| if (!is_valid_real(param[4], &threshold_lo_)) { | |
| PRINT_INPUT_ERROR("threshold should be a real number.\n"); | |
| } | |
| if (!is_valid_real(param[5], &threshold_hi_)) { | |
| if (!is_valid_real(param[4], &threshold_low_)) { | |
| PRINT_INPUT_ERROR("threshold_low should be a real number.\n"); | |
| } | |
| if (!is_valid_real(param[5], &threshold_high_)) { |
| if (!is_valid_real(param[4], &threshold_lo_)) { | ||
| PRINT_INPUT_ERROR("threshold should be a real number.\n"); | ||
| } | ||
| if (!is_valid_real(param[5], &threshold_hi_)) { | ||
| PRINT_INPUT_ERROR("threshold should be a real number.\n"); |
There was a problem hiding this comment.
| PRINT_INPUT_ERROR("threshold should be a real number.\n"); | |
| PRINT_INPUT_ERROR("threshold_high should be a real number.\n"); |
| @@ -238,7 +241,7 @@ void Active::process( | |||
| } | |||
| } | |||
| write_uncertainty(step, global_time, uncertainty); | |||
| if (uncertainty > threshold_) { | |||
| if (threshold_hi_ > uncertainty && uncertainty > threshold_lo_) { | |||
There was a problem hiding this comment.
| if (threshold_hi_ > uncertainty && uncertainty > threshold_lo_) { | |
| if (threshold_high_ > uncertainty && uncertainty > threshold_low_) { |
I have not encountered this situation personally, but I can see it potentially being an issue. My main concern is if it should be set by the user, or be an internal fixed parameter. As I said in my other comment, I think it's difficult to set a good value for an internal parameter however. |
|
@elindgren I agree. The value of the upper cutoff sometimes requires some experience to choose. However, this functionality mainly references the exploration part of dpgen. Therefore, I think we might be able to implement this functionality here as well. BTW, don't forget to modify the |
I think you need to take care of this. We cannot merge a broken code. |
fix the issue "If the system has exploded, unphysical structures may be saved since no upper bound is set on the uncertainty threshold."
Summary
The active learning workflow can be used with
active <interval> <has_velocity> <has_force> <threshold_lo> <threshold_hi>Modification
Rename
threshold_tothreshold_lo_, and addthreshold_hiOthers