aiidateam / aiida-wannier90

AiiDA plugin for the Wannier90 code
https://aiida-wannier90.readthedocs.io
Other
9 stars 14 forks source link

Include a parser for the input file #99

Open espenfl opened 4 years ago

espenfl commented 4 years ago

Sometimes, especially when running Wannier90 in library mode it is useful to read the input file.

We should consider to include a parser for this file. As an initial approach we might reuse what is in the aiida-vasp plugin: https://github.com/aiida-vasp/aiida-vasp/blob/develop/aiida_vasp/parsers/file_parsers/win.py and https://github.com/aiida-vasp/aiida-vasp/blob/develop/aiida_vasp/parsers/vasp2w90.py.

giovannipizzi commented 4 years ago

Hi Espen, thanks! Actually, I have been discussing with @jimustafa in the past weeks, who has written a module called wannier90-utils, and he's happy to share the work. I was planning to write back an issue here these days, but since you just opened one, I'll report here.

What about the idea of creating a single library, merging the work of @jimustafa, yours, and possibly other work (e.g. some code here to generate .win) into a single library, that we collectively maintain, and that is AiiDA independent? We have been following a similar approach with Quantum ESPRESSO with the qe-tools.

Great if you could give a look to the library of Jamal and see if you like it, so we can decide how to proceed with that, and if we want to make that as the starting point for a common set of utilities for W90 (and how to proceed, e.g. move it to a different GitHub organisation, clarify who is going to maintain it, ...)

Feedback appreciated also by the other people involved, e.g. @greschd @normarivano @AntimoMarrazzo @qiaojunfeng

espenfl commented 4 years ago

That is great and makes total sense. I suggest that we use this library and make an AiiDA interface (should be placed in aiida-wannier90) that offer an easy way to e.g. dump StructureData to atoms etc. Also, this should have some tests that ensure this wrapper works up updates in wannier90-utils etc.

qiaojunfeng commented 4 years ago

I think this is a great idea, to have an aiida-independent package for parsing Wannier90 related files, not only input file but also bands data files, etc. Previously, I personally wrote a simple win file parser for retrieving crystal structure, but I didn't have the time to add more features and make it stable enough. So if we could unify similar efforts into a single, well-tested package, that would be useful in many cases. Great to hear any ongoing plan about this.

espenfl commented 4 years ago

From our side, we are happy to drop whatever we have in the aiida-vasp plugin as that is the bare minimum. It is probably easier for us just to call whatever routines are defined in aiida-wannier90 and then in aiida-wannier90 you decide how you want to do this.

Would it be possible to start the work on implementing these function in aiida-wannier90 now (like write_win from a Dict, get_win into a Dict etc.)? In the meantime these can just point to some temporary parsers so what we have some work. I suspect, merging a fully fledged joined parser is going to take time and it would be nice to have something that works in the meantime.

jimustafa commented 4 years ago

Looks like there is some consensus that a collaborative effort to develop a generic wannier90 I/O library would be useful to the community. Not sure about the best starting point; if wannier90-utils makes sense, then I would be happy to take some direction on what additional functionality is needed to work with the aiida-wannier90 package. After a review of wannier90-utils, we can begin a discussion of issues/features there.

greschd commented 4 years ago

Hi @jimustafa,

Sorry for the long radio silence, I only just got the chance to look at wannier90-utils in some detail. It looks awesome, thanks for writing & suggesting it!

I have a few comments w.r.t. making this work with aiida-wannier90:

What are your backwards-compatibility requirements for wannier90-utils? Since we will have to make some interface changes to accommodate all use cases, I could also see pulling the I/O related code from the different sources (aiida_wannier90/io, w90_utils/io, aiida_vasp/parsers) into a separate project w90-io. Both aiida-wannier90 and wannier90-utils could switch to using that at their own pace once we have finalized the interface there.

espenfl commented 4 years ago

Thanks for the suggestions @greschd. I do not think the stuff in the VASP plugin is that usable once we can an interface to wannier90-utils. Also it would make total sense for us to ditch any wannier90 specific code in favour of a library.

In VASP we also have parsevasp which handles the parsing (makes sense to have this separate as other software might use it) and then we have wrappers in aiida-vasp which just access the necessary functions. Maybe we should do something similar in the aiida-wannier90? Also, if we consider to revisit the parsers, we also have a fairly generic parsing system in aiida-vasp which is not really VASP specific at all (node composer etc.). Maybe it can be reused if we are looking at a larger overhaul of the parsing done in aiida-wannier90? Not sure, just mentioning it.

greschd commented 4 years ago

Yeah, I'm vaguely aware of the generic interface in parsevasp - long-term it's probably a good idea to use a similar approach (maybe sharing the very base code) to parsing. For now though, I'd probably just take the existing parsing code and put it in a separate library.

espenfl commented 4 years ago

In fact, the generic parser stuff is in aiida-vasp. The actual parsing is done in parsevasp and that is rather brute force and not general. Okey, I agree with your suggestion.

jimustafa commented 4 years ago

Responding to the comments made by @greschd:

Regarding the routines for writing wannier90-related files... yes, let me take some time to compare those in wannier90-utils and aiida-wannier90.

For those wannier90-utils I/O routines that take a file path (ie, str) as an argument, it should be possible to allow for generators and file objects in a backwards-compatible way.

Currently, wannier90-utils is not considered completely stable (only v0.1.0); so, it is likely OK to incorporate backwards-incompatible changes that may be needed to make the library more usable.

A separate library could be a good route, especially in terms of the "do one thing, and do it well" UNIX philosophy. In fact, wannier90-utils is mostly just I/O routines; so, the repo could be copied to a new repo, and have all non-I/O-related code removed. At that point, we would be free to make any interface changes we wanted.

giovannipizzi commented 4 years ago

Hi all, just to keep the momentum going, is there agreement in developing a stable wannier90-utils library starting to @jimustafa and extending with functionality from other plugins? Would anybody have the time to work actively on it? If so, please say it so and we should then organise a Skype/Zoom meeting to discuss the details and then get started with the implementation

greschd commented 4 years ago

For me both approaches would work, either a separate I/O library, or adopting wannier90-utils after some slight interface changes. I'd say this is up to @jimustafa, there are certainly trade-offs to both (some additional overhead in maintaining two libraries, and making it a bit more difficult to neatly integrate the two).

I have capacity to do some interface design and PR reviews, but not to do the bulk of the work :neutral_face:

jimustafa commented 4 years ago

I would be happy to take responsibility for the active development of either approach, although, a bit more inclined to just develop a separate I/O library. A good starting point could be cloning wannier90-utils and striping the non-I/O-related routines, and modifying the existing interface according to what we decide. It seems that reproducing the functionality of wannier90, such as computing the centers and/or the spread, may not add much value, and can create additional maintenance (see jimustafa/wannier90-utils#14). Really, it seems that a library for interfacing with wannier90 could be separate from a library of routines for constructing MWLFs and computing their properties.

espenfl commented 4 years ago

Yes, I support going for a separate library (or sub-libraries if need be). I can follow and do PR reviews, possible contribute with some smaller parts, but I have no capacity to do the majority, unfortunately.

greschd commented 4 years ago

we should then organise a Skype/Zoom meeting

@giovannipizzi I think that's a great idea, do you want to coordinate that?

giovannipizzi commented 3 years ago

Unfortunately I won't be able to coordinate much at least until the end of the year - if someone wants to take the lead and prepare the scaffolding for the library (@jimustafa?) that'd be great. I can create a repository e.g. here under aiida-team for now, and if things go well in case we even move it directly under wannier-developers?

espenfl commented 2 years ago

@giovannipizzi @jimustafa What do we think of this one? Can we make this happen? We can maybe contribute a bit from the AiiDA-VASP side as well.

giovannipizzi commented 2 years ago

I suggest to schedule a group to work on this at the upcoming Wannier developer meeting in Trieste, Italy, next May! Here's te link: http://indico.ictp.it/event/9851/ (Because before it will be hard for me to find time - of course, if you want to work on it before, that's great!)

Would it work for you? Pinging also @jimustafa @qiaojunfeng to check if they are interested.

jimustafa commented 2 years ago

Hey all. Glad there is still interest. I can try to get something going...

espenfl commented 2 years ago

@jimustafa Excellent. If we could start something rather soon that would be awesome. What we are looking for is to move out Wannier90 input parsers out of the AiiDA-VASP plugin so that it is maintained in a proper place. I understand such a thing does not exist in the plugin? Is this something you can add? Here some some things that we have already that does not really work: https://github.com/aiida-vasp/aiida-vasp/blob/develop/aiida_vasp/parsers/vasp2w90.py. We would for instance like to call something linke parse_win and get something useful back. In addition it would be nice to be able to write such a file as well.

giovannipizzi commented 2 years ago

I suggest to work in a new repo (or modifying Jamal's wannier-utils). If we want, we can then move it either in this organization, or even better maybe in the wannier-developers organization (we'll need to see if the name is clear enough).