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

fix bug for sys_idx dependent trust level and add option for model_devi_job #786

Merged
merged 6 commits into from
Jul 11, 2022

Conversation

Cloudac7
Copy link
Contributor

@Cloudac7 Cloudac7 commented Jul 7, 2022

As mentioned in #746, there is a bug in #609 due to not converting str indexes to int ones. So the bug was fixed here. Also, the author of the issue mentioned that it would be more proper to select trust level for each iteration. So now, iteration dependent trust level for force and virial was supported. For example, just setting model_devi_f_trust_hi in model_devi_jobs:

"model_devi_jobs": [
    {
        "sys_idx": [0, 1], 
        "temps": [50,100],
        "press": [1.0,2.0],
        "trj_freq": 10,
        "nsteps": 1000,
        "ensemble": "npt",
        "model_devi_f_trust_hi": 0.30,
        "_idx": "00"
    }
]

However, it might be difficult to adapt different trust level for sys_idx selected in each iteration. So now, dict could be used to set the trust level in format like:

"model_devi_f_trust_hi": {
    "0": 0.160, 
    "1": 0.150
}

@Cloudac7
Copy link
Contributor Author

Cloudac7 commented Jul 7, 2022

Sorry, document change not pushed. Would be updated later.

dpgen/generator/arginfo.py Show resolved Hide resolved
"model_devi_dt": 0.002,
"model_devi_skip": 0,
"model_devi_f_trust_lo": [0.050, 0.050],
"model_devi_f_trust_hi": {"1": 0.150, "0": 0.150},
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand here - why not directly use an integer?

Copy link
Contributor Author

@Cloudac7 Cloudac7 Jul 7, 2022

Choose a reason for hiding this comment

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

To make sure passing the original unit test. What only in need here is just to check whether the list and dict parser work and thus I made two examples. As a matter of fact, it should be refactored as normal usage, setting different values for each system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it seems that the level does not affect the test result. I would change it to different values.

Copy link
Member

Choose a reason for hiding this comment

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

I mean why {"1": 0.150, "0": 0.150} is not {1: 0.150, 0: 0.150}... I don't see the reason to use a str.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, just in prevention of misleading, because most keys are string. Seems not to be so necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Oh you are right. JSON only allows string as the key... (Python does not)

Comment on lines 2475 to 2482
if cur_job.get('model_devi_f_trust_lo'):
f_trust_lo = cur_job.get('model_devi_f_trust_lo')
else:
f_trust_lo = jdata['model_devi_f_trust_lo']
if cur_job.get('model_devi_f_trust_hi'):
f_trust_hi = cur_job.get('model_devi_f_trust_hi')
else:
f_trust_hi = jdata['model_devi_f_trust_hi']
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not write it as

f_trust_lo = cur_job.get('model_devi_f_trust_lo', jdata['model_devi_f_trust_lo'])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If written like this, on the condition that trust level only supplied in each model_devi_job, it might result in IndexError because of the lack of global trust level. It would be confusing for user to set global trust level when local ones set.

dpgen/generator/run.py Outdated Show resolved Hide resolved
* fix unexpected 0 bug
* fix document not supporting typing bug
* change value for trust level in unittest
@wanghan-iapcm wanghan-iapcm merged commit 8ed4e2f into deepmodeling:devel Jul 11, 2022
njzjz added a commit to njzjz/dpgen that referenced this pull request Jul 23, 2022
wanghan-iapcm pushed a commit that referenced this pull request Jul 24, 2022
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