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

Make multiplying with unit_SI optional #309

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

AlexanderSinn
Copy link
Member

@AlexanderSinn AlexanderSinn commented Feb 25, 2021

This PR adds the ability to change the unit conversion when reading in data.
By specifying units='SI' in OpenPMDTimeSeries(), the functions get_particle() and get_field() will output pure SI data.
'SI_u' (default) will give SI data but with normalized particle momentum, like how it is currently.
'raw' will ignore all unit_SI attributes. (but will still multiply the particle data with weighting if appropriate)
With this approach more unit systems can be added to read_species_data() in the future.

Related to #209

@AlexanderSinn AlexanderSinn changed the title Make multiplying with units_SI optional Make multiplying with unit_SI optional Feb 25, 2021
@ax3l ax3l requested review from ax3l and RemiLehe March 4, 2021 03:42
Copy link
Member

@ax3l ax3l left a comment

Choose a reason for hiding this comment

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

Looks great to me, thank you for the PR!

@RemiLehe what do you think?

Also: should we add a test/example showcasing this?

openpmd_viewer/openpmd_timeseries/main.py Outdated Show resolved Hide resolved
openpmd_viewer/openpmd_timeseries/main.py Outdated Show resolved Hide resolved
openpmd_viewer/openpmd_timeseries/main.py Outdated Show resolved Hide resolved
@ax3l ax3l added the feature label Mar 4, 2021
AlexanderSinn and others added 4 commits March 4, 2021 12:21
Co-authored-by: Axel Huebl <[email protected]>
Co-authored-by: Axel Huebl <[email protected]>
Co-authored-by: Axel Huebl <[email protected]>
Co-authored-by: Axel Huebl <[email protected]>
@ax3l ax3l mentioned this pull request Apr 7, 2021
data += offset
# - Return momentum in normalized units
elif component_name in ['ux', 'uy', 'uz' ]:
elif component_name in ['ux', 'uy', 'uz' ] and units == 'SI_u':
Copy link
Member

Choose a reason for hiding this comment

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

@RemiLehe has a good idea how we can separate the normalized momentum vs. SI aspects:
He will introduce alongside u(x|y|z) a p(u|y|z) naming that will not divide by mass * c: #315

With that, we can remove the SI_u logic from this PR and separate both aspects well.

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

Successfully merging this pull request may close these issues.

2 participants