move atomicrex interface development to contrib #65
Conversation
Pull Request Test Coverage Report for Build 607362118
💛 - Coveralls |
|
|
||
| ## Class defined for future addition of other codes | ||
| ## Not sure which functionality (if any) can be extracted yet, but a similar pattern is followed in other pyiron modules | ||
| class PotentialFittingBase(GenericJob): |
There was a problem hiding this comment.
Like it, probably once #51 is in, we want to include it here. Can you already move that class to something like pyiron_contrib/atomistic/job/potential_fitting.py?
| self.output = Output() | ||
|
|
||
| """ | ||
| def to_hdf(self, hdf=None, group_name=None): |
There was a problem hiding this comment.
Should call super().to_hdf(hdf=hdf, group_name=group_name) and similarly pass those to the other to_hdf. Otherwise it should work.
| return False | ||
|
|
||
| def to_xml_element(self): | ||
| xml = ET.Element(f"{self.prop}") |
There was a problem hiding this comment.
f-strings are python 3.6+ and we nominally support down to 3.4, so we either remove them or bump our python requirements. Am open to both.
There was a problem hiding this comment.
I am fine with dropping 3.4 and 3.5 in particular for pyiron_contrib.
| class Poly(InputList): | ||
| def __init__(self, identifier, cutoff, species): | ||
| super().__init__(table_name=f"Poly_{identifier}") | ||
| self.identifier = identifier |
There was a problem hiding this comment.
These will end up as elements in the InputList. If you don't want this call object.__setattr__ like here to define them as instance variables. Accessing them afterwards works with the normal syntax though.
If you want them as elements this is fine though (and will also take care of saving them).
| structure.from_hdf(hdf, group_name = g) | ||
| self.append(structure) | ||
|
|
||
| def to_ase_db(self, db): |
There was a problem hiding this comment.
See #51 again. It's ok unify those a bit later though.
|
https://github.com/pyiron/pyiron_base/blob/master/pyiron_base/job/generic.py#L1475 - this is the function that should be overwritten. |
|
I created a small mybinder environment for testing https://github.com/jan-janssen/pyiron-atomicrex |
|
So the executable part seems to be working now, it only fails to store data in the HDF5 file: Basically it does not like the structures to be stored in the |
We have plans for this, see here, but have nothing concrete right now to revive it. For now we either store it as in the StructureContainer or use #51, which I'll merge later today. |
|
@Leimeroth Can you explain me the difference between |
I wanted to inherit ARStructureList from StructureList and reuse StructureList when trying to extend the code for Lammps. But since the plan is to move to StuctureContainer it is probably unnecessary to keep the StructureList. |
Atomicrex - hdf5 fixes
|
@Leimeroth To my understanding both the executable issue and the HDF5 issue is fixed now. What I would be interested now is an example which uses atomicrex to fit a potential and then uses the resulting potential for a very simple LAMMPS calculation. If it helps you can create a fork of https://github.com/jan-janssen/pyiron-atomicrex and just upload the notebook there then we can easily test it on mybinder. |
Running the job works now, but it is not possible to load it using Project.load(). |
|
@pmrv I guess the current issue with the HDF5 storage is that the classes are derived from InputList, so when they are reloaded from the HDF5 they are just input lists and not the specific class. In certain cases like the potential class it is not even possible to initialise the class with an empty dictionary, as depending on the type of potential - Lennard Jones or EAM - the classes have different utility functions. So the question is: Do we already have utility functions in the Inputlist to restore the original class which was previously derived from the inputlist? |
Changed output
Atomicrex rework factories
|
The hdf storage of the general input still doesn't work and I can't figure out why. input.atom_types and input.fit_algorithm both inherit from InputList but are not written to hdf correclty, while it works completely fine for potential and fit properties of the structures, which do the same as far as I can tell. Could you maybe have a look at it @pmrv or @jan-janssen. |
|
I can't straight-away see what the problem is, but I'm not sure if Will have a deeper look tomorrow. |
|
@Leimeroth Can we merge it now? And can you upload the corresponding notebook to https://github.com/pyiron/pyiron_potentialfit/tree/master/day_2 ? then we can use it to build a test environment of the workshop |
|
@Leimeroth Currently I use the |
|
Something just broke the hdf storage in my notebook. I think it is the change from InputList to Datacontainer that cause this. |
|
@pmrv Can you take a look at this? @Leimeroth I guess for the moment it is fine to use |
Can have a look in the afternoon. It seems that keys with decimal places in them break the HDF5 interface. |
@Leimeroth Can you try if this error persists with this branch? |
As discussed I will continue the development of an atomicrex interface to pyiron in pyiron_contrib.
Essentially there are 2 points where I still need help before this works like other pyiron jobs imo.
The probably more difficult one is storing and loading the jobs in hdf5. I used InputLists whenever possible so I hope it isn't too hard.
The second point is how to find the executable in an intelligent way. Right now it fails because the automatic search path uses the module path and splits it at the wrong place for the atomicrex module if it stays in contrib/atomistic to find it in the resources. So I think either place it one level higher, redefining that function or changing the pattern in the resources directory is the solution and I am not sure which is the better way to go.