units as parameter in read_*

[tfrederiksen]

Although sisl works with eV as the default energy unit, users may just use sisl as an interface and keep original units (or some other choice).

Here a suggestion for ORCA. I could extend to the other interfaces too.

[zerothi]

When you brought up the MHz question, I was also thinking about this.

I can't really figure out if this is a good idea or not. There are soo many places where values have units. And I am worried it might end up being confusing, and error-prone.

[tfrederiksen]

I think the use case is quite clear for the HFI. Here some people will want to work with "MHz" as this is a more natural energy scale without having to worry about making conversion errors if we force it to "eV" at the interface.

[tfrederiksen]

It also makes it explicit for the user what the units are (both in the source file as well as after import in sisl).

[tfrederiksen]

OK, I see your point that it may not be relevant to implement this everywhere. But at least for read_hyperfine_coupling?

[zerothi]

Yeah, probably we should do it. But... :)

If we are going to do this, we need to figure out how to do it for multiple return values, say it also returned some coordinate (length), how should we deal with that?

[tfrederiksen]

Do you have an example of that? I guess we can stick to sisl defaults if it is about reading nc files etc with lots of different data inside.

[zerothi]

Do you have an example of that? I guess we can stick to sisl defaults if it is about reading nc files etc with lots of different data inside.

A simple example would be to read the Hamiltonian, it uses the coordinates and an energy.
So you might request Ry energy, and then typically one would use Bohr for the coordinates.
I know this is a contrieved example, but later on when some return functions returns complicated things multiple units could be valuable. E.g. read_scf in siesta, it basically returns all information in the SCF cycle, and there we might add dipole information etc.

[tfrederiksen]

But reading this one gets a sisl object Hamiltonian. Here I would not enable unit conversion. I think it should be limited to "low-level" reading only. And maybe on a case-by-case consideration?

[tfrederiksen]

Eventually, one could have multiple parameters: unit_energy, unit_length etc.

[zerothi]

Yes, but I would consider read_scf as low-level. So it might be worthwhile to get the interface correct.

We could imagine a decorator enabling the units argument, which then processes it to a dict. Something like this:

@units_handling # not the best word
def read_hyperfine_interaction(...)


sile.read_hyperfine_interaction(units=("MHz", "Ang"))

then internally the units would be a dict with entries:

units = {"energy": "MHz", "length": "Ang"}

or similar?

[tfrederiksen]

We could also always add the quantity to the parameter?

[tfrederiksen]

Wrong button, sorry!

[zerothi]

We could also always add the quantity to the parameter?

you mean that users should do units={"energy": "MH"}?

[tfrederiksen]

We could also always add the quantity to the parameter?

you mean that users should do units={"energy": "MH"}?

I mean that this PR should be changed to unit_energy="eV".

[zerothi]

I am not in favor of having unit_* for all variants. ;) That seems extremely verbose where a dict could handle it.

[tfrederiksen]

OK, then what about accepting both str and dict as inputs for unit?

[zerothi]

OK, then what about accepting both str and dict as inputs for unit?

Yes, this was my idea, here I don't think we need anymore (other than changing it to units for future consistency). Then a decorator can change the way it is intercepted to streamline the conversions for arguments.

[zerothi]

My idea of the decorator was to convert the units argument to a dict always

[codecov]

Codecov Report

Attention: Patch coverage is 98.82353% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 86.87%. Comparing base (ffc5539) to head (6dd18de).
Report is 1 commits behind head on main.

Files	Patch %	Lines
src/sisl/unit/base.py	92.85%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #726      +/-   ##
==========================================
+ Coverage   86.86%   86.87%   +0.01%     
==========================================
  Files         399      399              
  Lines       50948    50998      +50     
==========================================
+ Hits        44257    44306      +49     
- Misses       6691     6692       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tfrederiksen]

My idea of the decorator was to convert the units argument to a dict always

Should we also allow calls without the keyword, i.e., read_energy("MHz")? Then the decorator becomes a little complex, no?

[zerothi]

My idea of the decorator was to convert the units argument to a dict always

Should we also allow calls without the keyword, i.e., read_energy("MHz")? Then the decorator becomes a little complex, no?

good question, no, I think it would be wise to forcefully ask the user to write units=, good point!

[tfrederiksen]

I've given the decorator a try. What do you think?

[tfrederiksen]

I'm facing some problems that I think is related to the other decorators. Wouldn't it be simpler with just a function like this?

def read_hyperfine_coupling(units="eV"):
    units = sisl.unit.unit2dict(units)

Then it is perhaps also easier to see what is going on, as compared to

@units2dict
def read_hyperfine_coupling(units="eV"):
    assert isinstance(units, dict)

[zerothi]

fine for me, I'll probably rename unit2dict to something like: serialize_units_arg for clarity

[zerothi]

The decorator requires an inspection pass. So lets just keep it as is. :)

EDIT: I mean, as you suggest :)

[zerothi]

Should we merge this now, and take it from there?

[tfrederiksen]

OK, maybe the docstrings just need an update with units : {str, dict, list, tuple}?

[zerothi]

OK, maybe the docstrings just need an update with units : {str, dict, list, tuple}?

Yeah, later we should change this to use typing constructs. :)

[zerothi]

But I think this is fine

[tfrederiksen]

OK, I think it is ready now.

[zerothi]

Merged! Thanks a lot @tfrederiksen