Skip to content

Modification to the Units module#673

Open
rjtanner wants to merge 9 commits intoPrincetonUniversity:mainfrom
rjtanner:unit_mod
Open

Modification to the Units module#673
rjtanner wants to merge 9 commits intoPrincetonUniversity:mainfrom
rjtanner:unit_mod

Conversation

@rjtanner
Copy link
Copy Markdown
Contributor

Prerequisite checklist

  • My code follows the Athena++ Style Guide
  • My change requires a change to the documentation.
  • I have updated the documentation in the Wiki accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Please review the CONTRIBUTING.md file for detailed guidelines.

Description

Major changes:

  1. I reworked the units module so that it now explicitly works from a "unit basis" (something that was implicitly used in the original units module). The unit basis is used to convert from physical units to code units. The updated units module is backwards compatible with the previous version.
  2. I modified the units module to make it easier to add additional units, and included a number of new basis units.

Minor changes:

  1. I modified the module unit to include more preset unit systems.
  2. Some minor changes to the names of a few constants to agree with the FITS standard for unit names.
  3. I included a function that will output the units_override dictionary needed for reading in HDF5 files using yt.
  4. The mean atomic weight can now be set in the input file.

Testing and validation

This doesn't change anything "internal" to Athena, but only facilitates the interface between the input file and the problem file.

I included an example problem and input file that demonstrates how to use units.

To-do

The wiki page on units would need to be updated. I can write something up for that.

@buildbot-princeton
Copy link
Copy Markdown
Collaborator

Can one of the admins verify this patch?

@tomidakn tomidakn requested a review from changgoo May 30, 2025 05:11
@tomidakn
Copy link
Copy Markdown
Contributor

I assigned @changgoo because I think he originally implemented this. @changgoo , can you review it?

@changgoo
Copy link
Copy Markdown
Contributor

changgoo commented Jun 6, 2025

Sorry for taking too long to review this. Before going through the new additions, the renaming of variables is violating the style guid.

I personally has no preference in variable style, but the current units.cpp tries to conform with the suggested style guide

  • Variable names (snake_case): all lower case, words separated by underscore, e.g. iso_csound.

@felker
Copy link
Copy Markdown
Contributor

felker commented Oct 1, 2025

ok to test

Copy link
Copy Markdown
Contributor

@tomidakn tomidakn left a comment

Choose a reason for hiding this comment

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

It looks fine to me. Some variables names do not follow the style but I think Gconst and H(hydrogen) are acceptable. But I would like to leave the final decision to @changgoo .

I appreciate if you could write up the Wiki documentation and send it to me so that I can update it.

Comment thread src/pgen/unit_test.cpp
}
phydro->u(IDN,k,j,i) = den;
phydro->u(IM1,k,j,i) = vel;
phydro->u(IM2,k,j,i) = vel/2;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In C, real numbers should be written like 2.0, 3.0.

Comment thread src/units/units.cpp
if (unit == "km/s") {
code_cgs_ = Constants::km_s_cgs*value;
} else if (unit == "m/s") {
code_cgs_ = 100*value;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

100.0

Comment thread src/units/units.cpp
} else if (unit == "g") {
code_cgs_ = value;
} else if (unit == "kg") {
code_cgs_ = 1000*value;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

1000.0

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.

6 participants