-
Notifications
You must be signed in to change notification settings - Fork 8
Add a Unit class to EXP coefficients [WIP] #160
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
base: devel
Are you sure you want to change the base?
Conversation
- The Unit class is a vector of tuples of name (ascii), unit type (ascii), and value (float). - This vector is serialized in HDF5 coefficient files as a list of tuples - Manipulators and accessors are provided as part of Python bindings to C++ member functions - The gravitational constant is set on read from a Coefs instance and used to set the gravitational constant in BiorthBasis. - The value of G=1 by default (exp). The user is responsible for setting the units in a Coefs instance using the setUnit() call, one for each physical unit including the gravitational constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a Unit class to the EXP coefficient system to provide unit information for gravitational simulations. The main goal is to track physical units and the gravitational constant for proper scaling of potential and force calculations in both EXP and pyEXP.
Key changes:
- Introduces a
Unit
struct to store unit metadata (name, unit string, value) in HDF5-compatible format - Implements unit management methods in the
Coefs
base class for setting, reading, and writing units - Integrates gravitational constant scaling throughout the basis evaluation functions
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.
Show a summary per file
File | Description |
---|---|
pyEXP/CoefWrappers.cc | Adds Python bindings for unit management and warns users to set units before writing HDF5 files |
expui/Coefficients.cc | Implements core unit functionality including HDF5 serialization and gravitational constant retrieval |
expui/Coefficients.H | Defines the Unit struct and declares unit management methods in the Coefs class |
expui/CoefStruct.H | Adds gravitational constant field to coefficient structures |
expui/BiorthBasis.cc | Integrates gravitational constant scaling in all basis evaluation methods |
expui/BasisFactory.H | Adds gravitational constant member to basis factory |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
…mit header inclusion for HighFive
Summary
Unit
class provides the three fields(name as char[16], unit as char[16], value as float)
.Coefs
base class maintains astd::vector<Unit>
container, written to coefficient files as an HDF5dataset
h5py
as an array ofdtype([('name', 'S16'), ('unit', 'S16'), ('value', '<f4')])
EXP integration
Unit
instance("G", "none", 1.0f)
is the default. Applications can call thesetUnit(name, unit, value)
member function to add units. The intent is that theunit
field is one of the standard astronomically common units, e.g. as used byastropy
. However,Unit
does no checking. I suggest the standard name fieldsLength
,Time
,Mass
,Velocity
andG
but there is no name checking.setUnit
will check for thename
in the array and replace units and values. The matching is case insensitive. One call is needed for each unit.Coefs
class will populate theUnit
array on read.Coefs
class provides agetUnits()
member which returns a vector of tuples as<std::string, std::string, float>
and agetGravConstant()
member that returns the value of the gravitational constant onlypyEXP integration
setUnit()
,getUnits()
andgetGravConstant()
are bound topyEXP.coefs.Coefs
interfacepyEXP::Basis
will scale potential and force quantities with the current value of the gravitational constantpyEXP
will be warned/encouraged to provide unit data if no units have been set.Gala integration strategy
Now that I understand the Gala/astropy units concept, we should be able to use both
UnitSystem
andSimulationUnitSystem
depending on the user's need. One might be begin by using thepyEXP.Coefs.getUnits()
to pull vector of tuples that describe the user's units. There are two clear-cut cases:G=1
with a suppliedMass
andLength
unit. In that case, Gala can create aSimulationUnitSystem
object.G!=1
with eitherMass
,Length
, andVelocity
orMass
,Length
, andTime
. In that case, Gala can create aUnitSystem
and the value ofG
will be handled correctly bypyEXP
. There is an obvious internal check: the implied value ofG
by the three supplied physical units should match (with some tolerance) the supplied value ofG
. Note:G
is a required field inexp
in the current implementation. So this should be unambiguous; e.g. the user can not specifyMass
,Length
, andTime
and not specifyG
. Alternatively, I could attempt to do the unit parsing on thepyEXP
side. But that seems like inappropriate duplication of existing code.Checks
Unit
vector is correctly created byCoefs
Unit
vector is correctly serialized into HDF5Units
dataset is correctly read byh5py
andEXP
Units
are correctly reread when the coefficient dataset is extended during anexp
simulationG=1
on existing examples forSpherical
andCylindrical
G
are correctly handed by thepyEXP.basis.Basis
classes