Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Adjust the call paramters for the seekpath_structure_analysis function accordingly. #4

Merged
merged 1 commit into from
Apr 18, 2020

Conversation

hongyi-zhao
Copy link
Contributor

See #3
and
aiidateam/aiida-quantumespresso@ec24908

seekpath_structure_analysis: expand parameter input into keywords

So, adjust the call method for the seekpath_structure_analysis function accordingly.

and
aiidateam/aiida-quantumespresso@ec24908

`seekpath_structure_analysis`: expand parameter input into keywords

So, adjust the call method for the seekpath_structure_analysis function accordingly.
@hongyi-zhao hongyi-zhao changed the title Adjust the call method for the seekpath_structure_analysis function accordingly. Adjust the call paramters for the seekpath_structure_analysis function accordingly. Apr 16, 2020
@sphuber
Copy link

sphuber commented Apr 18, 2020

I am not sure if you guys actually tested this, but I think this won't work. The seekpath_parameters is a Dict node, but just expanding it won't work. Each individual keyword needs to be a separate node.
So if before it was:

parameters = Dict(dict={
       ' with_time_reversal': True
        'reference_distance': 0.02
}
seekpath_structure_analysis(structure, parameters

now it should be

seekpath_structure_analysis(structure, with_time_reversal=Bool(True), reference_distance=Float(0.02))

@hongyi-zhao
Copy link
Contributor Author

hongyi-zhao commented Apr 18, 2020

I tested it with this example and it seems work.

But it you are right, the call method is more complex now, we should judge the datatype of the arguments, converting them correctly before passing them into function.

Regards

qiaojunfeng added a commit that referenced this pull request Apr 18, 2020
@qiaojunfeng
Copy link
Collaborator

I tested and it worked, because the kpoints_distance_for_bands in line 225

kpoints_distance_for_bands = self.inputs.controls.get(
'kpoints_distance_for_bands',
self.ctx.protocol['kpoints_distance_for_bands']
)
seekpath_parameters = orm.Dict(
dict={'reference_distance': kpoints_distance_for_bands}
)

is the one provided in the workflow inputs
spec.input(
'controls.kpoints_distance_for_bands',
valid_type=orm.Float,
default=lambda: orm.Float(0.01),
help='Kpoint mesh density of the resulting band structure.'
)

which is always of type orm.Float, that's why it works.

A problem is line 222 where self.ctx.protocol['kpoints_distance_for_bands'] is of type float, but this line won't be executed since some time ago I added a default to the kpoints_distance_for_bands in the spec.input. For safety reason, I wrap the self.ctx.protocol['kpoints_distance_for_bands'] in orm.Float (see commit f901818), though maybe this line should be removed in the future.

@hongyi-zhao
Copy link
Contributor Author

hongyi-zhao commented Apr 18, 2020

I still cannot figure out why you use the most simplified seekpath parameters for that. The seekpath package has many parameters for the dict defined by you:

 seekpath_parameters = orm.Dict( 
     dict={'reference_distance': kpoints_distance_for_bands} 
 ) 

But you only use one of them.

Regards

@qiaojunfeng
Copy link
Collaborator

I still cannot figure out why you use the most simplified seekpath parameters for that. The seekpath package has many parameters for the dict defined by you:

 seekpath_parameters = orm.Dict( 
     dict={'reference_distance': kpoints_distance_for_bands} 
 ) 

But you only use one of them.

Regards

That's simply because reference_distance is used for calculating the band structure. I haven't met with problems of finding primitive cell by seekpath, so I just use the default parameters of seekpath.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants