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

Except unit uni pint quantity. #527

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

RubelMozumder
Copy link
Collaborator

No description provided.

@RubelMozumder
Copy link
Collaborator Author

Involved with the PRs

if units_key in self.data.keys() and self.data[units_key] is not None:
dataset.attrs["units"] = self.data[units_key]
units = self.data.get(units_key, None)
units = str(units) if isinstance(units, pint.Unit) else units
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use the pint.UnitRegistry (like in NOMAD)? See how I am using it in the pynxtools-xps here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, using just if isinstance(units, pint.Unit) does not seem to work.

If I do the following

import pint

units = "m"
print(isinstance(units, pint.Unit))

that prints False, so obviously this unit check does not work as intended.

pint.Unit is just a class to implement a unit, not something to be used directly to check a string is a pint unit. We need to use pint.UnitRegistry, like in NOMAD.

@lukaspie lukaspie force-pushed the FixErrorIfUnitInPintQuantity branch from e6eb16e to bf3f4fb Compare January 30, 2025 16:21
Copy link
Collaborator

@lukaspie lukaspie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RubelMozumder I rebased this branch on top of master to make it reviewable.

I don't think your check if isinstance(units, pint.Unit) works, see below.

if units_key in self.data.keys() and self.data[units_key] is not None:
dataset.attrs["units"] = self.data[units_key]
units = self.data.get(units_key, None)
units = str(units) if isinstance(units, pint.Unit) else units
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, using just if isinstance(units, pint.Unit) does not seem to work.

If I do the following

import pint

units = "m"
print(isinstance(units, pint.Unit))

that prints False, so obviously this unit check does not work as intended.

pint.Unit is just a class to implement a unit, not something to be used directly to check a string is a pint unit. We need to use pint.UnitRegistry, like in NOMAD.

@RubelMozumder RubelMozumder marked this pull request as draft January 30, 2025 17:56
@RubelMozumder RubelMozumder force-pushed the FixErrorIfUnitInPintQuantity branch from 91e624d to fb0a864 Compare January 30, 2025 18:48
ureg.Unit(units)
except pint.errors.UndefinedUnitError as exc:
massage = (
f"Units provided for path: '{path}@units' are not valid."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
f"Units provided for path: '{path}@units' are not valid."
f"Units '{units}' provided for path: '{path}@units' are not valid."

try:
ureg.Unit(units)
except pint.errors.UndefinedUnitError as exc:
massage = (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
massage = (
message = (

f"Units provided for path: '{path}@units' are not valid."
f" Please provide a valid unit."
)
raise InvalidDictProvided(massage) from exc
Copy link
Collaborator

@lukaspie lukaspie Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall this raise an error or simply be logged as a warning? The file can be written without this wrong unit, right?

Suggested change
raise InvalidDictProvided(massage) from exc
logger.warning(message, exc_info=True)

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