Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1rst draft for retrieving parameters unit #299

Closed
wants to merge 1 commit into from

Conversation

eimerej
Copy link

@eimerej eimerej commented Oct 15, 2020

I'm just sharing the code that I'm currently using to retrieve the units of the parameters.
Just in case it could help. It's at draft state, but working well for me.

In this approach, the __getitem__ of basic_object returns an object_attribute object with unit and values instead of the values alone.

@ErlendHaa
Copy link
Contributor

Thanks for the contribution!

The main problem with units, is how to include them in the Python API. In a dlis file, and in our core, an attribute is essentially this:

struct attribute {
   dl::ident               label; 
   dl::value_vector        value;
   dl::units               units;
   dl::representation_code reprc; // type of value, e.g. str, int
   dl::uvari               count; // number of values
}

However in python we try to be convenient by treating each attribute as an dict item: {label : value}.
I.e. this is what allows you to do write channel.long_name rather than than having to type channel.long_name.value etc. At the time that abstraction seamed reasonable as reprc, count and units was thought to be of little interest. (Note that units is present for all attributes, and for the majority units doesn't make any sense, like channel.long_name.) In hein sight this might have been a mistake. And we are now left with an API where units doesn't fit in nicely and removing the attribute abstraction all together means breaking the entire interface.

Im a bit hesitant to introduce a partial solution like parameter.units because it does not scale. I have looked a bit at other options, like bringing in a units library like pint [1] so we can return the value with the units directly from e.g. parameters.values. That would of course have to be opt-in. We can also do something similar without pint. However, I would need to experiment with it a bit more to find an ergonomic interface and hopefully don't break the entire interface, but unfortunately there are other issues that are higher on my agenda at the moment.

[1] https://pint.readthedocs.io/en/stable/

@ErlendHaa
Copy link
Contributor

Hi,

Thanks for the contribution. We took the liberty of including some of your changes in c65a6f2. The latest release, which includes these changes are out and you can now get the units as described here: #171 (comment)

I'm going to close this PR now, but as I mentioned in the issue, we have yet to make a good interface for units. Feel free to re-open this PR with new changes to how units are handled nicely if you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants