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

More convenient way to convert python objects to sisl #282

Closed
pfebrer opened this issue Nov 12, 2020 · 3 comments · Fixed by #284
Closed

More convenient way to convert python objects to sisl #282

pfebrer opened this issue Nov 12, 2020 · 3 comments · Fixed by #284

Comments

@pfebrer
Copy link
Contributor

pfebrer commented Nov 12, 2020

I think siles are great for standarizing the input of files into sisl, but there's no such infrastructure for python objects.

Maybe the creation and register of "converters" would help. Each converter would look something like this:

class AseAtomsConverter:

    def from(ase_obj):
        ...code to convert ASE object into Geometry.
        return geometry

    def to(geometry):
         ...code to convert Geometry into ASE.
         return ase_obj

And it would be registered into the corresponding class (in this case, Geometry). Then, the from and to dispatchers would be implemented, and one could do:

geometry.to("ase")
# or
geometry.to.ase()

One thing that I would particularly like is that I wouldn't need to call the from dispatcher. The class would check on call whether it is being passed an object that has been registered as "convertible" and use it. That is:

sisl.Geometry(ase_obj)

would work as the current

sisl.Geometry.fromASE(ase_obj)

Edit: Maybe sisl.Geometry.from(ase_obj) is enough so that we don't interfere in normal geometry creation

This would probably give the ability for a class to implement as many parsers as wanted without overpopulating with methods (which was one of your concerns for Geometry).


PS: I would love a converter from strings because it would save me some repetitive code:

sisl.Geometry("mygeom.fdf")
# would be equivalent to:
sisl.get_sile("mygeom.fdf").read_geometry()
@pfebrer
Copy link
Contributor Author

pfebrer commented Nov 12, 2020

By the way, just to give a bit more background: this thought was because I would like to accept different python objects as values for a given setting in the viz module.

E.g.: a geometry setting should be able to accept anything that can be parsed into a geometry. If this was implemented, the parsing would be as easy as sisl.Geometry.from(<value of the geometry setting>). This would help reducing the number of settings that need to be defined in a very clean way.

@zerothi
Copy link
Owner

zerothi commented Dec 10, 2020

I think this would be a nice addition.

It would solve a couple of problems:

  1. allow users to add parsers
  2. remove the need for toXYZ and fromXYZ for all available formats.

How to exactly implement this is something we should consider.
My initial thought is something like this geom.from(obj) where the from is some kind of special dispatcher.

This dispatcher should react to types. So something like:

def __call__(self, *args, **kwargs):
    obj = args[0]
    return self._dispatchs[type(obj)](*args, **kwargs)

and these dispatch methods are just simple methods (no need for dispatchers). However, with actual AbstractDispatch we could do more magic. But that could be handled easily.

Now comes the problematic part. By using type we have a hard time ensuring compatibility with say geom.from.ase. This requires a tuple at register that defines the type and the str equivalent for the look-up table.
While this is a minor issues it may easily cause problems downstream if two objects are called the same, say sisl.Geometry and xyz.Geometry which isn't too far-fetched. So the register function should probably warn about overwriting.

@zerothi
Copy link
Owner

zerothi commented Dec 10, 2020

Oh... second problem... from is a reserved word... We can't use that.

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 a pull request may close this issue.

2 participants