Support equivalent and identical units in capgen and ccpp-prebuild#656
Merged
Conversation
… feature/equivalent_units
… feature/equivalent_units
… vs m+2 s-2) and equivalent units - skip any conversions
…(m2 s-2 vs m+2 s-2) and equivalent units - skip any conversions
climbfuji
commented
Apr 14, 2025
climbfuji
requested review from
dustinswales,
gold2718,
grantfirl,
mkavulich,
mwaxmonsky and
peverwhee
as code owners
climbfuji
added
capgen
Collaborator
|
Previous comment from @peverwhee #571 (review) |
…4182025 Check that equivalent units did change the answer.
dustinswales
approved these changes
Apr 22, 2025
mkavulich
requested changes
Apr 29, 2025
Collaborator
mkavulich
left a comment
mkavulich
left a comment
There was a problem hiding this comment.
Just some minor comments/suggestions
| end if | ||
| ! Check values in data array2 | ||
| write(error_unit,'(a,e12.4)') 'In unit_conv_scheme_1_run: checking min/max values of data array 2 to be approximately ', target_value2 | ||
| if (minval(data_array2)<0.99*target_value2 .or. maxval(data_array2)>1.01*target_value2) then |
Collaborator
There was a problem hiding this comment.
This probably doesn't matter, but do you know why we chose 1% deviation as the "acceptable" level of difference? That seems very large for a unit conversion.
Collaborator
Author
There was a problem hiding this comment.
If the unit conversion isn't working as expected, the result will be off by a factor of 1E3 / 1E-3. Therefore, any arbitrary value that is smaller than that works.
Co-authored-by: Michael Kavulich <kavulich@ucar.edu>
Co-authored-by: Michael Kavulich <kavulich@ucar.edu>
mkavulich
approved these changes
May 1, 2025
… feature/equivalent_units
Collaborator
Author
|
@mkavulich I resolved the conflicts and tests all pass. Can we merge so that I don't have to do this again? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds logic to support equivalent and identical units in both capgen and ccpp-prebuild.
Note. This PR builds on top of #571 (credits @dustinswales). It contains all its commits, therefore we can either keep #571 until this PR is merged, or close it as superseded by #656.
m2 s-2is identical tom+2 s-2). Internally, bothcapgenandccpp-prebuildconvert the former to the latter. Forcapgen, this happens deep down in the unit conversion logic. Ifcapgenfinds that two units are identical after parsing them, it returns a(None, None)unit conversion that tells the VarCompatibilityObject that the variables are equivalent w.r.t. units. Forccpp-prebuild, this happens when translatingcapgenmetadata toccpp-prebuildmetadata. The outcome is the same.m2 s-2is equivalent toJ kg-1). This is handled differently for the two code generators. Whilecapgenskips the unit conversion step (VarCompatObj says variables are equivalent, hence no conversion required),ccpp-prebuilduses an identity transformation{var} = {var}.As a result, the
unitsmetadata can now be written asm2 s-2orm+2 s-2. Both are acceptable and dealt with by the two code generators.The
ccpp-prebuildimplementation is simpler than thecapgenimplementation, because of the interface between thecapgen-provided metadata from the parser and the metadata used byccpp-prebuild, and also because theccpp-prebuildimplementation is more of a workaround (yes, prebuild needs to go).User interface changes?: No
Fixes #570
Testing:
test removed: none
unit tests: all pass, including doc tests; added several doc tests for the new capabilities
system tests: all pass; extended
capgenandccpp-prebuildtests to cover the new featuresmanual testing: ran all unit and system tests on my laptop