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

Fix ValueAtPoints data type #652

Merged
merged 2 commits into from
Feb 20, 2024
Merged

Conversation

tjhei
Copy link
Contributor

@tjhei tjhei commented Feb 17, 2024

max_values_in_array should be an integer not a double.

Also update schema files.

@gassmoeller gassmoeller added the ready to merge Pull request is ready to merge. May be waiting for tests to complete or other reviews. label Feb 17, 2024
@tjhei
Copy link
Contributor Author

tjhei commented Feb 17, 2024

FYI: @danieldouglas92

Copy link

codecov bot commented Feb 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (13a43a6) 92.82% compared to head (beaa8f3) 92.82%.
Report is 6 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #652   +/-   ##
=======================================
  Coverage   92.82%   92.82%           
=======================================
  Files         105      105           
  Lines        7176     7176           
=======================================
  Hits         6661     6661           
  Misses        515      515           
Files Coverage Δ
...eanic_plate_models/temperature/half_space_model.cc 98.43% <100.00%> (ø)
...es/oceanic_plate_models/temperature/plate_model.cc 100.00% <100.00%> (ø)
...ucting_plate_models/temperature/mass_conserving.cc 98.62% <100.00%> (ø)
source/world_builder/types/value_at_points.cc 100.00% <ø> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13a43a6...beaa8f3. Read the comment docs.

Copy link

github-actions bot commented Feb 17, 2024

Benchmark Main Feature Difference (99.9% CI)
Slab interpolation simple none 1.012 ± 0.004 (s=445) 1.013 ± 0.005 (s=446) -0.0% .. +0.2%
Slab interpolation curved simple none 1.019 ± 0.007 (s=453) 1.018 ± 0.006 (s=433) -0.2% .. +0.0%
Spherical slab interpolation simple none 1.169 ± 0.011 (s=392) 1.170 ± 0.008 (s=380) -0.1% .. +0.2%
Slab interpolation simple curved CMS 1.058 ± 0.004 (s=461) 1.062 ± 0.004 (s=391) +0.3% .. +0.5%
Spherical slab interpolation simple CMS 1.549 ± 0.009 (s=299) 1.553 ± 0.010 (s=284) +0.1% .. +0.4%
Spherical fault interpolation simple none 1.171 ± 0.009 (s=393) 1.173 ± 0.008 (s=378) +0.0% .. +0.4%
Cartesian min max surface 2.326 ± 0.017 (s=216) 2.327 ± 0.028 (s=173) -0.3% .. +0.4%
Spherical min max surface 7.217 ± 0.031 (s=58) 7.221 ± 0.023 (s=69) -0.2% .. +0.3%

@tjhei
Copy link
Contributor Author

tjhei commented Feb 17, 2024

Hm, doesn't compile on the tester?!

@MFraters
Copy link
Member

hmm, so it turns out that rapidjson doesn't support size_t for this part of the conversion directly:

//! Constructor for int value.
explicit GenericValue(int i) RAPIDJSON_NOEXCEPT :
data_()
{
data_.n.i64 = i;
data_.f.flags = (i >= 0) ? (kNumberIntFlag | kUintFlag | kUint64Flag) : kNumberIntFlag;
}
//! Constructor for unsigned value.
explicit GenericValue(unsigned u) RAPIDJSON_NOEXCEPT :
data_()
{
data_.n.u64 = u;
data_.f.flags = (u & 0x80000000) ? kNumberUintFlag : (kNumberUintFlag | kIntFlag | kInt64Flag);
}
//! Constructor for int64_t value.
explicit GenericValue(int64_t i64) RAPIDJSON_NOEXCEPT :
data_()
{
data_.n.i64 = i64;
data_.f.flags = kNumberInt64Flag;
if (i64 >= 0)
{
data_.f.flags |= kNumberUint64Flag;
if (!(static_cast<uint64_t>(i64) & RAPIDJSON_UINT64_C2(0xFFFFFFFF, 0x00000000)))
data_.f.flags |= kUintFlag;
if (!(static_cast<uint64_t>(i64) & RAPIDJSON_UINT64_C2(0xFFFFFFFF, 0x80000000)))
data_.f.flags |= kIntFlag;
}
else if (i64 >= static_cast<int64_t>(RAPIDJSON_UINT64_C2(0xFFFFFFFF, 0x80000000)))
data_.f.flags |= kIntFlag;
}
//! Constructor for uint64_t value.
explicit GenericValue(uint64_t u64) RAPIDJSON_NOEXCEPT :
data_()
{
data_.n.u64 = u64;
data_.f.flags = kNumberUint64Flag;
if (!(u64 & RAPIDJSON_UINT64_C2(0x80000000, 0x00000000)))
data_.f.flags |= kInt64Flag;
if (!(u64 & RAPIDJSON_UINT64_C2(0xFFFFFFFF, 0x00000000)))
data_.f.flags |= kUintFlag;
if (!(u64 & RAPIDJSON_UINT64_C2(0xFFFFFFFF, 0x80000000)))
data_.f.flags |= kIntFlag;
}
//! Constructor for double value.
explicit GenericValue(double d) RAPIDJSON_NOEXCEPT :
data_()
{
data_.n.d = d;
data_.f.flags = kNumberDoubleFlag;
}
//! Constructor for float value.
explicit GenericValue(float f) RAPIDJSON_NOEXCEPT :
data_()
{
data_.n.d = static_cast<double>(f);
data_.f.flags = kNumberDoubleFlag;
}

We could use a unsigned int or int64_t instead, or cast it to a int64_t where it is used, or try to implement it.

tjhei and others added 2 commits February 19, 2024 12:28
max_values_in_array should be an integer not a double.

Also update schema files.
@MFraters
Copy link
Member

@tjhei are you oke with this solution, turining the size_t into uint64_t?

@tjhei
Copy link
Contributor Author

tjhei commented Feb 19, 2024

Yes, sorry didn't get time to update yet

@MFraters
Copy link
Member

I updated it for you :)

@@ -117,7 +117,7 @@ namespace WorldBuilder
prm.declare_entry("density", Types::Double(3300),
"The reference density of the subducting plate in $kg/m^3$");

prm.declare_entry("plate velocity", Types::OneOf(Types::Double(0.01),Types::Array(Types::ValueAtPoints(0.01, std::numeric_limits<size_t>::max()))),
prm.declare_entry("plate velocity", Types::OneOf(Types::Double(0.01),Types::Array(Types::ValueAtPoints(0.01, std::numeric_limits<uint64_t>::max()))),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do you use uint max for doubles?

Copy link
Member

Choose a reason for hiding this comment

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

That contains the max number of elements:

ValueAtPoints::ValueAtPoints(const double default_value_,
size_t max_values_in_array_,
std::vector<Point<2>> default_points_)

@tjhei tjhei mentioned this pull request Feb 20, 2024
@tjhei tjhei merged commit 0b03a7e into GeodynamicWorldBuilder:main Feb 20, 2024
34 checks passed
@tjhei tjhei deleted the update-schema2 branch February 20, 2024 03:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull request is ready to merge. May be waiting for tests to complete or other reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants