lsmo-epfl / aiida-zeopp

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

Pr add count block and units #39

Closed danieleongari closed 5 years ago

danieleongari commented 5 years ago

See commit for explanation.

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 232


Changes Missing Coverage Covered Lines Changed/Added Lines %
aiida_zeopp/parsers/network.py 5 6 83.33%
<!-- Total: 5 6 83.33% -->
Totals Coverage Status
Change from base Build 227: -0.2%
Covered Lines: 1148
Relevant Lines: 1245

💛 - Coveralls
ltalirz commented 5 years ago

P.P.S. Once this is decided, I think there are still a few changes needed (e.g. the information on the units should move inside the parsers, and they should return the appropriate dictionaries). But let's wait until the main decision is taken.

danieleongari commented 5 years ago

What you propose is very elegant solution, however my purpose was not for visualisation but only to inform the user when he reads the output Dict. Your solution is still missing this purpose: so I would keep both.

In general, the two solutions are used in other plugins: 1) have the units in the final part of the label (e.g., typically in Zeopp, Density_g/cm^3) 2) have the units separated (e.g., typically in Raspa, Density_average, Density_dev, Density_unit).

I recently pointed to @yakutovicha the problem of having it sometimes spelled as unit and sometime units, that makes it even more confusing: https://github.com/yakutovicha/aiida-raspa/issues/26

I decided to implement it here as option (2) not to break the output API for Density, but simply adding an extra informative key.

ltalirz commented 5 years ago

If that is also the approach followed by others, then I'm fine with this.

I suggest two changes (which I will take care of myself):

Sounds good?

danieleongari commented 5 years ago

Thanks, it does sound good to me, only be careful to use <quantity>_unit, i.e., the singular (https://github.com/yakutovicha/aiida-raspa/issues/26)!

Daniele

ltalirz commented 5 years ago

Ok. I was thinking plural but I agree that singular is the more natural. Will use the singular.

danieleongari commented 5 years ago

please refer to https://github.com/ltalirz/aiida-zeopp/pull/42