Unify master jobs

[samwaseda]

Part of the reason why this PR is failing is because InteractiveWrapper does (not) do something that ParallelMaster and SerialMaster do (not). So far I failed to see what's exactly the difference, but in the meantime I try to put them closer, because a lot of functions are repeated, even though they all derive from GenericMaster.

Main changes

• input defined in GenericMaster
• collect_output defined in GenericMaster, which now collects everything in output/generic
• Make SerialMaster compatible with interactive jobs
• SerialMaster should launch the next calculation only when convergence_goal returns a job

[jan-janssen]

self[-1] is not the same as self[0]

[samwaseda]

I understand that they don't have the same effect, but I don't see why we should have different definitions for SerialMaster and ParallelMaster here

[jan-janssen]

The behavior of the SerialMaster and the ParallelMaster differ, that is the reason why it is implemented in different classes.

[samwaseda]

The behavior of the SerialMaster and the ParallelMaster differ, that is the reason why it is implemented in different classes.

I know that they are not the same, so I'm also not trying to have one class out of them.

[samwaseda]

My biggest problem is that I've been working on this PR for more than half a year now without being able to figure out what's the right solution. This has been a huge problem for me because I cannot easily move on as it's part of the fundamental SPHInX functionality. Since it looks like the combination of SPHInX and ParallelMaster seems to be working, I'm trying to see where the difference between ParallelMaster and InteractiveWrapper comes from.

[samwaseda]

@pmrv and @ligerzero-ai : Here I am adding docstrings to some of the classes we talked about yesterday. Let me know what you think and feel free to make suggestions.

[jan-janssen]

For me this pull request tries to do too many things at once, can we split it in multiple pull request? For example we could have one to unify the write_input functions and so on.

[samwaseda]

For example we could have one to unify the write_input functions and so on.

This PR just moves the location of write_input without changing literally anything in functionality. Feel free to open another PR when this is done, but the change you are suggesting has nothing to do with this PR.

[samwaseda]

Can I merge this one? @jan-janssen

Have you tested the notebooks in pyiron_atomistics ? I mean in particular the phonopy notebook and the pipeline notebook?

They are not included in the integration tests?

[jan-janssen]

They are not included in the integration tests?

Notebooks are not included in integration tests

[samwaseda]

For me this pull request tries to do too many things at once, can we split it in multiple pull request?

Yes I can, but your suggestion makes only sense if the changes are entangled or there are conceptually multiple changes, which is not the case here. From what I am seeing right now, you are pointing out individual changes which have nothing to do with each other. In the end, for the changes I'm proposing here it shouldn't matter whether there are n times small PR, each of which takes a shorter time to review, or one time this PR which takes n times the amount of time.

[jan-janssen]

@samwaseda I created three examples how the changes in this pull request can be split in small changes, which are easy to review: #914 , #915 and #916 . I hope this helps to demonstrate what I meant with separate pull requests, rather than one big one.

[samwaseda]

Whatever.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.