changed charge convention

[jmmshn]

The sign convention of the net charge parsed by Vasprun should be changed.

The current value of the parsed charge for Vasprun.structure is more positive if NELECT is higher.
If we used the electron -> negative charge convention then this expression should have the other sign

[shyuep]

Unittest needed.

[coveralls]

Coverage decreased (-0.001%) to 63.589% when pulling 5f2ce21 on jmmshn:charge_sign into 72d6e8b on materialsproject:master.

[jmmshn]

The unit tests already exist but used the wrong convention?
@mkhorton and I had a chat and think we should use negative throughout to avoid these issues in the future.
The unit tests have been updated to reflect this.

[shyuep]

Thanks.

[mturiansky]

This convention change silently broke the code that I use. I think it's a bit irresponsible to change conventions like this without properly warning your users, especially for such a large, dynamic, and widely used library such as this. The only reason I was able to find out that something was wrong with my code was because I had old data that wasn't matching; if this were not the case, this change might have gone unnoticed, resulting in bad calculations and data. This is very concerning. @shyuep @mkhorton

[shyuep]

I think the original implementation was wrong to begin with. It is perfectly reasonable that a negative charge corresponds to an electron (otherwise we violate almost all conventions used in physics and chemistry?). This PR fixes that problem.

As with all cases, we document the changes in the release. But perhaps it would be useful to add a warning to users that the convention has changed if there is a charge. @jmmshn can you just add a warning?

[jmmshn]

@shyuep ok I'll add a warning. Thanks for bringing this up @mturiansky.

[mturiansky]

I think the original implementation was wrong to begin with. It is perfectly reasonable that a negative charge corresponds to an electron (otherwise we violate almost all conventions used in physics and chemistry?). This PR fixes that problem.

I can't speak to why this convention was chosen for the original implementation, but it was and people developed on top of that using that convention. I can't imagine I'm the only one who used it, and I suspect by adding a warning someone else may be surprised to find that it has changed.

I understand you can't put a warning for every tiny change, but a convention change like this, where previous code could work just fine within the old convention, seems like a good case for one.