Phonon workflow - generate displacements only in generate_structure()

[jan-janssen]

Before they were generated during initialisation.

[liamhuber]

I don't have any objections to this change, but I also don't understand the benefit?

At first I thought maybe it was so that you could update workflow.structure and then get up-to-date results from generate_structure, but since self.phonopy is already populated using the init structure (via _phonopy_unit_cell), I guess not?

With the big disclaimer that I'm probably just totally misunderstanding what's going on here, it seems to me that if it's worth moving self.phonopy.generate_displacements down into PhonopyWorkflow.generate_structures, maybe it's worth moving everything after self.structure = structure in PhonopyWorkflow.__init__ there too?

[jan-janssen]

I don't have any objections to this change, but I also don't understand the benefit?

It is primarily to simplify debugging when analysing which step consumes how many resources. I was surprised to see how efficient the symmetry analysis is until I realised that it already happens during the initialisation.

[liamhuber]

I don't have any objections to this change, but I also don't understand the benefit?

It is primarily to simplify debugging when analysing which step consumes how many resources. I was surprised to see how efficient the symmetry analysis is until I realised that it already happens during the initialisation.

👍 sounds good.

With the big disclaimer that I'm probably just totally misunderstanding what's going on here, it seems to me that if it's worth moving self.phonopy.generate_displacements down into PhonopyWorkflow.generate_structures, maybe it's worth moving everything after self.structure = structure in PhonopyWorkflow.__init__ there too?

I would appreciate a response to this too. This is kind of a big problem IMO; Workflow objects return a class instance, and PhonopyWorkflow exposes the public, settable property structure, but if this property is updated there is no change to the downstream functionality:

from ase.build import bulk
from atomistics.workflows import PhonopyWorkflow

workflow = PhonopyWorkflow(structure=bulk("Al", cubic=True))
workflow.structure = bulk("Cu", cubic=True)
structure_dict = workflow.generate_structures()
print(structure_dict["calc_forces"][0].symbols[0])
>>> Al

?? !! ??

(Edited to add the print value)

[jan-janssen]

I would appreciate a response to this too. This is kind of a big problem IMO; Workflow objects return a class instance, and PhonopyWorkflow exposes the public.

I guess my recommendation would be to make structure a private variable rather than a public one, to follow the general scheme of atomistics to always (1) create the workflow, (2) generate the task_dict , (3) execute it with a calculator and (4) analyse the results. I worked on a first set of specs in #173 but this is still a draft.

[liamhuber]

I guess my recommendation would be to make structure a private variable rather than a public one,

Yes, that sounds reasonable. The catch is that you then need to be extremely rigorous about making sure that all the class properties are private/read-only, not just structure, and not just for this particular workflow.

to follow the general scheme of atomistics to always (1) create the workflow, (2) generate the task_dict , (3) execute it with a calculator and (4) analyse the results. I worked on a first set of specs in #173 but this is still a draft.

Perhaps I'm just mis-reading you, but it sounds like you are suggesting to impose user behaviour by fiat rather than encode it in the data structures. As long as workflows are class instances, I'm not convinced it's fair to demand that users just remember that they're not allowed to treat them as such. One needs to either adopt a truly functional framework or, as suggested above, be extremely careful to always make sure all the relevant class attributes are locked down hard.

I don't have any changes to request to this PR, but given the problems it highlights so strongly I'm also not comfortable approving it, so you'll need to ping someone else for review if you want more than "comment" status.

[jan-janssen]

Yes, that sounds reasonable. The catch is that you then need to be extremely rigorous about making sure that all the class properties are private/read-only, not just structure, and not just for this particular workflow.

I leave this to a separate pull request - for now I opened an issue #227

Perhaps I'm just mis-reading you, but it sounds like you are suggesting to impose user behaviour by fiat rather than encode it in the data structures. As long as workflows are class instances, I'm not convinced it's fair to demand that users just remember that they're not allowed to treat them as such. One needs to either adopt a truly functional framework or, as suggested above, be extremely careful to always make sure all the relevant class attributes are locked down hard.

I feel we can expect our users to not modifying private variables of the classes in atomistics. Also covered in issues #227 .

I don't have any changes to request to this PR, but given the problems it highlights so strongly I'm also not comfortable approving it, so you'll need to ping someone else for review if you want more than "comment" status.

Comment is fine for me.