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

Half_life of isotopes return float, but unit differs depending on which isotopes is requested #94

Closed
jeremy-jimmy opened this issue Jan 25, 2023 · 11 comments

Comments

@jeremy-jimmy
Copy link

from mendeleev import element, isotope

iode_131 = isotope("I", 131)
cesium_137 = isotope("Cs", 137)

iode_131.half_life => 8.0249 as a float (it's 8.0249 day)
cesium_137.half_life ==> 30.04 as a float (it's 30.04 year)

I feel this data should be implemented using the python module pint to return Quantities and the the user could transform as expected depending on usage

Another quicker approach would be to normalize the data in one specific unit and to precise it in the docstring

@lmmentel
Copy link
Owner

Thanks for reaching out @jeremy-jimmy. You are raising a very valid point and the UX of isotopes could be a lot better. Python layer that mendeleev has right now in this case reflects the db schema where values, units and uncertainties are stored separately (see this section in the docs for more info). You should be able to fetch time units as strings under the half_life_unit attribute.

Really good suggestion on using pint for handling values and units together. I think most of the necessary parts are there. Would you be interested in taking a first pass at it?

@jeremy-jimmy
Copy link
Author

Thanks for such a fast answer.

I don't know how come I didn't spot the half_life_unit attribute. Knowing that I feel that my feature become a bit useless.

Pint is a great package to handle units, but implementing it in mendeleev would provoke a major regression for users in term of user interface.

Maybe your choice of data architecture is the easiest after all.

Do you agree with me ?

@lmmentel
Copy link
Owner

No worries, the docs could be a bit clearer on those properties.

I think it's an interesting idea to integrate pint and I would be interested in exploring it further. I know that Mendeleev's sister project in Julia is putting more emphasis on handling units on par with values.

The might be ways of introducing pint without causing an incompatibility. We could have a new attribute or accessor for value with units that would return a pint Quantity object or sth similar.

I have limited capacity to look into it myself but I'm open to contributions so go ahead if you want to experiment with it and see how it could look. I'm more than happy to support if that would be helpful.

@YgorSouza
Copy link

I think it's an interesting idea to integrate pint and I would be interested in exploring it further. I know that Mendeleev's sister project in Julia is putting more emphasis on handling units on par with values.

I have been pondering the same thing as I port your package to Rust. On the one hand, it would be great for correctness, but on the other hand, I don't want to force users to take a dependency that they might not want, so I want to cover both use cases, but that is hard to do and might lead to even more confusion.

Generally the advice is to include the unit in the function/property name if the return value is just a number, like melting_point_k, fusion_heat_kj_per_mol etc, and then we could add functions without the suffix that return the strongly-typed quantities, but that would be a breaking change, of course. In the case of the half-life, I guess it would be something like half_life_in_unit? Indicating to the user that they have to get the unit from somewhere else.

@lmmentel
Copy link
Owner

lmmentel commented Feb 2, 2023

Thanks for pitching in @YgorSouza, interesting take. I wonder what would be a good default when accessing values that have units. I like the idea of including the unit in the attr name. I was also thinking about an unit-aware instance of the Element class where instead of returning values (primitive types) it all the attributes would return Quantities. Then you, as a user could either import Element or ElementwUnits early and use the desired quantities based on what you need.

@YgorSouza what do you think about adding a section in the readme linking to your RUST version of mendeleev?

@kalvdans
Copy link
Collaborator

kalvdans commented Feb 2, 2023

My opinion is to just return the half-life in seconds and let the user convert to a suitable unit for presentation.

@YgorSouza
Copy link

@lmmentel Creating a new type to avoid breaking changes could work as well. I'll keep thinking about it for now and see if I can find similar libraries for ideas. About the Readme, I suppose it could help someone find the Rust crate, but I don't want to be misleading. The Rust crate has a lot less data at the moment and the API is not necessarily compatible, since I'm trying to go for idiomatic Rust and keeping the crate as lightweight as possible, rather than copying the Python API. Also, the Julia project you mentioned isn't in the Readme right now, is it? I guess there could a section with links to similar packages in other languages, so people could take a look at their code and exchange ideas between them.

@lmmentel
Copy link
Owner

lmmentel commented Feb 6, 2023

Great,if you find some inspirational examples around the web please share also here since it might be the push we need to start implementing.

I appreciate that your rust implementation is at an earlier stage and might follow a different path but I think it would be nice to let people know that it exists 🙂 I honestly didn't think about mentioning other related projects in the readme until I read your comment so that's why I didn't put the julia one yet but I probably will.

@YgorSouza
Copy link

So I finally found out about the Rust API guidelines, and this is what they recommend for this specific scenario: https://rust-lang.github.io/api-guidelines/type-safety.html#newtypes-provide-static-distinctions-c-newtype

I think it's pretty nice, as it makes the API really obvious and hard to misuse, doesn't add too much boilerplate, and isn't opinionated about what physical quantity library users should use or depend on. So I'll probably switch to this on my next update. I don't know to what extent this is applicable to Python, and whether it is worth the breaking changes in a package that is already used by a lot of people, but I'll just leave it here for consideration.

In the case of the half-life, I think I'll return them all in the same unit, and maybe add some helper methods to convert/display them in the most appropriate unit according to the order of magnitude.

@lmmentel
Copy link
Owner

lmmentel commented Jun 1, 2024

After some idle time I managed to push the units related functionality a bit forward. Recent PR #160 updates half_life_unit values for isotopes so that they are compatible with pint out of the box. Previously the units were ambiguous to parse.

On a more general note I started moving metadata such as units and references (previously only in the docs layer) into the db layer so that they are available to the mendeleev package itself. This was the necessary update to make the transition towards dealing with units explicitly. This PR #156 has more details if you are curious about the change.

This means that it should be relatively straightforward to start experimenting with implementations that would include units. I started a thread in the discussions #162 Feel free to drop your thoughts there. I appreciate the feedback.

@lmmentel
Copy link
Owner

lmmentel commented Jun 1, 2024

Moving to discussions #162

@lmmentel lmmentel closed this as completed Jun 1, 2024
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

No branches or pull requests

4 participants