search

lsmo-epfl / aiida-zeopp

AiiDA plugin for zeo++
Other
5 stars 8 forks source link

Add 'visvoro' option #38

Closed ltalirz closed 5 years ago

ltalirz commented 5 years ago

resolving conflicts from #29

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 268

Totals Coverage Status
Change from base Build 264: 0.0%
Covered Lines:
Relevant Lines: 0
💛 - Coveralls
ltalirz commented 5 years ago

@pzarabadip I've tried to simplify your changes a bit. Can you give it a try with the new zeo++ executable?

One thing I don't really like is that this won't work with zeo++ 0.3 - the code runs but it does (obviously) not produce the expected output files if the visVoro option is specified. The question is: is this option obscure enough that people won't notice? Mentioning also @danieleongari for comment.

P.S. @pzarabadip I added you as a collaborator, so if you find something that needs fixing, you can directly update this branch.

ezpzbz commented 5 years ago

Thanks @ltalirz I just tested the plugin with new changes. Unfortunately, it does not work. The reason is that in order to keep the backward compatibility and also compatibility in zeo++, I only added the option to visVoro key in a way that it would only accept one user-specified prefix. However, the current implementation of plugin generates several prefixes which results in failure. If we only provide out.Visvoro as the output prefix, the problem should be solved.

ltalirz commented 5 years ago

@pzarabadip Thanks for the test and the instruction on what to change.

I've tried making the change now: https://github.com/ltalirz/aiida-zeopp/pull/38/commits/569b0d5bec55c18c70df8e7cc899cef18471e5de

Is this what is required?

ltalirz commented 5 years ago

After this PR is merged, I will make a new aiida-zeopp release

ezpzbz commented 5 years ago

@pzarabadip Thanks for the test and the instruction on what to change.

I've tried making the change now: 569b0d5

Is this what is required?

Thanks a lot @ltalirz . I just tested it also within my WorkChain and it works perfectly. I think we can merge it to the master.

ezpzbz commented 5 years ago

After this PR is merged, I will make a new aiida-zeopp release

During the testing within my WorkChain, I found a possible bug/issue. I will open an issue about it right now. Then, we would discuss if it would be necessary to fix it before the release or we can do it later. EDIT: I explained the issue in #45

ltalirz commented 5 years ago

@pzarabadip does this PR look good to you now?

ezpzbz commented 5 years ago

@ltalirz yeah, I think so. Thanks a lot.

ltalirz commented 5 years ago

Ok, I'll make a new release tomorrow