Feature/eckit stats #154

pmaciel · 2024-12-09T01:19:04Z

Mirco, I've ported 90% of the mir statistics functionality here, it's not a full PR to be merged before much discussion but rather a draft.

For MIR, the requirements were:

supporting finite and NaN missing values;
Calculate statistics in time (for weather statistics and climatotlogical studies) and/or in space (supporting a variety of interpolation methods);
Do online statistics, in partitioned objects that can be summed for global results (for parallel execution, although this was never taken advantage off);
Support comparisons for pgen (which is a lot); And
To support eccodes namespace "statistics" (grib_ls -n statistics), which is why there are spectral statistics too, but this isn't a requirement

I'm sure for other packages it will be different, let's see if they converge to something useful :-)

…, remove Auto as it is configurable

…ce/std deviation/energy norm

This reverts commit 52a2178. # Conflicts: # src/mir/data/MIRField.cc

…O_GRID

…op-convert, cppcoreguidelines-avoid-non-const-global-variables, readability-qualified-auto, bugprone-fold-init-type, bugprone-macro-parentheses

… of stddev

…alised non-POD object)

…-make-unique, modernize-use-auto, readability-container-data-pointer, readability-implicit-bool-conversion, readability-size-empty

…eature/eckit-stats

pmaciel · 2024-12-09T01:19:52Z

Again please do not merge this, before we have a wider discussion with other eckit devs.

codecov-commenter · 2024-12-09T01:26:40Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.74%. Comparing base (9d2807d) to head (74e8b31).

Additional details and impacted files

@@           Coverage Diff            @@
##           develop     #154   +/-   ##
========================================
  Coverage    63.74%   63.74%           
========================================
  Files         1066     1066           
  Lines        55360    55360           
  Branches      4086     4086           
========================================
  Hits         35289    35289           
  Misses       20071    20071

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

simondsmart

A few questions in here. Looks good overall.

simondsmart · 2024-12-09T09:54:20Z

src/eckit/stats/Comparator.cc

+namespace eckit::stats {
+
+
+static util::recursive_mutex* local_mutex           = nullptr;


Why does this need to be a recursive_mutex?

Can we not use the idiom, used elsewhere, with a static instance() method on the Factory type, and the mutex/map being local members. It's a bit cleaner than using posix once.

This is a wrapper around the eckit types that have been superceded by stl; There's a define above that controls this, and I've put it in one place only so it's easy to change.

I called it recursive mutex because that's what the stl calls it, and what the eckit::Mutex really is. But this can of course change, at the moment this is only for controlling access to the factories. On that note, the factories could move to a eckit-based implementation -- which I recently changed to behave correctly for concurrent access, but the change is non-trivial. I think this is what should likely happen first? In any case, happy to update

simondsmart · 2024-12-09T09:55:30Z

src/eckit/stats/Comparator.h

+protected:
+    // -- Members
+
+    const Parametrisation& parametrisation1_;


This feels a touch dangerous. For general use, can we be confident that the supplied Parametrisation will outlive the Comparator instance?

You're right, I'm not happy about this. Currently the Comparator is a class supporting comparing two statistics (say mean of two fields), but its use is quite specific to pgen-compare fields1 fields2 (which is in fact mir-compare). I probably should take the Comparator hierarchy out? I don't see that MultIO would use this.

simondsmart · 2024-12-09T09:56:08Z

src/eckit/stats/Distribution.cc

+namespace eckit::stats {
+
+
+static util::once_flag once;


Same comment RE instance() method rather than static global variables.

Indeed -- maybe factories refactoring come first though.

simondsmart · 2024-12-09T09:57:14Z

src/eckit/stats/Distribution.h

+namespace eckit::stats {
+
+
+class Distribution {


What does a Distribution do in this context. Not immediately obvious from the name - I think this needs some documentation comments.

You're right -- this is a class to generate noise onto a field according to specific distributions. It might not have a direct use here, but it is important functionality to support statistics testing (incl. downstream packages -- I use this in MIR to generate training data, although this isn't the greatest use). I used this to test the statistics when developing, and I haven't ported those tests as they should be done anew (they aren't useful as is)...

simondsmart · 2024-12-09T09:58:42Z

src/eckit/stats/Field.h

+using MIRValuesVector = std::vector<double>;
+
+
+class Field {


This is a bit odd. It isn't declared as an abstract base, but all of the methods are NOTIMP, and it doesn't carry any data. Huh?

Yes you caught on quickly -- this was meant for Mirco to have a look for MultIO first and see what the interface should look like. I've just pushed an update supporting owned and referenced fields, should be much more adequate for MultIO, and fully implemented.

This class is to serve as an argument to calculate field statistics in (for a definition of Field.) You're just too early on the PR as I intended to discuss it beforehand -- but now I've done a proper implementation that can be used in both MIR and MultIO

simondsmart · 2024-12-09T10:03:43Z

src/eckit/stats/Comparator.h

+
+    // -- Methods
+
+    virtual std::string execute(const Field&, const Field&) = 0;


std::string is a very non-obvious return type for a Comparator. Unless it is doing something weird.

Indeed this sholuld change -- it is a list of reasons why a comparison failed, and if empty is it a successful comparison. I should bring in more (a lot more) semantics here. Good catch!

simondsmart · 2024-12-09T10:05:54Z

src/eckit/stats/detail/Counter.cc

+    min_(std::numeric_limits<double>::quiet_NaN()),
+    max_(std::numeric_limits<double>::quiet_NaN()),
+    hasMissing_(hasMissing),
+    hasLowerLimit_(lowerLimit_ == lowerLimit_),


This looks odd. I assume it is essentially testing if things are NaN, or similar. But I worry that the equality comparison could be optimised away to true.

At this level, to support the statistics, they operate on real values and NaN if not assigned/not-definable (there's no "finite missing value"). The assumption here is the NaN support -- and you're right, this can be compiled out. I know that MIR and Atlas will fail in specific places to to similar optimisations. Maybe I should protect this with a static_assert? But the problem is widespread.

simondsmart · 2024-12-09T10:08:43Z

src/eckit/stats/method/MethodT.cc

+
+
+static const MethodBuilder<MethodT<detail::AngleT<double, detail::AngleScale::DEGREE, detail::AngleSpace::ASYMMETRIC>>>
+    __stats1("angle.degree.asymmetric");


Not sure I like the variable names starting with __. Those are supposed to be reserved symbols.

If you really want to ensure non-clashing, you can put them in an anonymous namespace?

simondsmart · 2024-12-09T10:10:40Z

src/eckit/stats/util/Mutex.h

+namespace eckit::stats::util {
+
+
+#if defined(ECKIT_GEO_ECKIT_THREADS)


This is a very odd, and somewhat worrying block of code. Why do we need to do special stuff. Why us std::mutex insufficient. And what is the logic here.

If it really is necessary, this needs to be heavily documented as to why (in comments).

This is due to the current threading model supprted in eckit (and the rest of our stacks), to be it's own implementation based on eckit::Mutex. This provides a #define that can change the threading model to stl, but for the stack we should do this in one go -- to me this is a separate discussion. It doesn't work, mixing the models. I've raised this issue over the last few years, several times, but gave up...

simondsmart · 2024-12-09T10:11:18Z

tests/stats/stats.cc

+
+constexpr double EPS = 1e-6;
+
+#define EXPECTV(a)                                      \


Consider adding these to Test.h

pmaciel and others added 30 commits December 5, 2016 18:44

Move mir::action::statistics::* to mir::stats::, make them not Action…

9c060b9

…, remove Auto as it is configurable

Work on Statistics

5e48e9a

Work on Statistics

a712a43

Work on Statistics

b3859fe

Work on Statistics

8ccc4f4

Work on Statistics comparisons

54013f0

Work on Statistics comparisons

d211233

Work on Statistics comparisons

a5e88ae

missing includes

b05421e

Tidy code

0b0419a

Cleanup some includes

87bdf92

Cleanup old code

da2082f

Cleanup

8ae993a

mir::data::Dimension remove dead code (and angle statistics)

cd2dc63

ECC-551: implemented resolution on calculation of the spectral varian…

89682b0

…ce/std deviation/energy norm

mir-statistics

d2da270

Fix license dates do we dont have to change every year

4a6f766

Abstract MIRValuesVector from std::vector (no container type change)

944e3a1

MIR-352, MIR-353, MIR-354: field statistics

e288ba4

MIR-352, MIR-353, MIR-354: field statistics

0d32d28

MIR-352, MIR-353, MIR-354: field statistics

11e2602

MIR-352, MIR-353, MIR-354: field statistics

dddc690

MIR-352, MIR-353, MIR-354: field statistics

049d8df

MIR-353: cleanup angles comparison

2912c80

MIR-352, MIR-353, MIR-354: field statistics

3071049

MIR-352, MIR-353, MIR-354: field statistics

f7f768d

Cleanup

fba383a

ECKIT-381: substitute eckit::ScopedPtr for std::unique_ptr

6d30695

Angle statistics/comparison performace

9155637

Fix mir-compare for reduced_ll

efa9a1d

pmaciel and others added 23 commits November 19, 2021 18:00

PGEN-426 Revert "ATLAS-301, MIR-454 FiniteElement options update"

fe0a36d

This reverts commit 52a2178. # Conflicts: # src/mir/data/MIRField.cc

Merge branch 'hotfix/1.9.7' into develop

2aadb50

Merge branch 'hotfix/1.9.7'

4973808

Merge branch 'release/1.10.0'

e7e9adb

MIR-559 k-nn interpolation (weighting) methods to match Metview GEO_T…

53cce12

…O_GRID

Merge branch 'feature/MIR-559' into develop

fc6a3db

hicpp-use-auto, llvm-include-order, llvm-qualified-auto, modernize-lo…

1da915f

…op-convert, cppcoreguidelines-avoid-non-const-global-variables, readability-qualified-auto, bugprone-fold-init-type, bugprone-macro-parentheses

Linting

17ff1c1

Merge branch 'release/1.11.0'

7625c38

Fix in interpolation-statistics method "sum", calculating sum instead…

cf15b3e

… of stddev

Cleanup

368e32b

Merge branch 'release/1.12.0'

279ffb2

clang-format (14)

a569ab7

Mode statistics correct construction (not a problem due to well-initi…

6f34418

…alised non-POD object)

modernize-concat-nested-namespaces, modernize-loop-convert, modernize…

044d6c3

…-make-unique, modernize-use-auto, readability-container-data-pointer, readability-implicit-bool-conversion, readability-size-empty

MIR-646 earthkit-regrid support with interpolationspec

50377f5

Merge branch 'feature/earthkit-regrid-interpolationspec' into develop

27c96c6

Cleanup

9bb79e8

eckit::stats

a5e007a

Merge remote-tracking branch 'eckit-stats/feature/eckit-stats' into f…

98f5f81

…eature/eckit-stats

eckit::stats

98e4ccf

eckit::stats

8557010

eckit::stats

1ecd639

pmaciel requested a review from MircoValentiniECMWF December 9, 2024 01:19

simondsmart reviewed Dec 9, 2024

View reviewed changes

pmaciel added 3 commits December 9, 2024 11:23

eckit::stats

e7c22fa

eckit::stats

055bc20

eckit::stats

74e8b31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/eckit stats #154

Feature/eckit stats #154

pmaciel commented Dec 9, 2024

pmaciel commented Dec 9, 2024

codecov-commenter commented Dec 9, 2024 •

edited

Loading

simondsmart left a comment

simondsmart Dec 9, 2024

pmaciel Dec 9, 2024

simondsmart Dec 9, 2024

pmaciel Dec 9, 2024

simondsmart Dec 9, 2024

pmaciel Dec 9, 2024

simondsmart Dec 9, 2024

pmaciel Dec 9, 2024

simondsmart Dec 9, 2024

pmaciel Dec 9, 2024

pmaciel Dec 9, 2024

simondsmart Dec 9, 2024

pmaciel Dec 9, 2024

simondsmart Dec 9, 2024

pmaciel Dec 9, 2024

simondsmart Dec 9, 2024

pmaciel Dec 9, 2024

simondsmart Dec 9, 2024

pmaciel Dec 9, 2024

simondsmart Dec 9, 2024

		namespace eckit::stats {


		static util::recursive_mutex* local_mutex = nullptr;


		// -- Methods

		virtual std::string execute(const Field&, const Field&) = 0;



		static const MethodBuilder<MethodT<detail::AngleT<double, detail::AngleScale::DEGREE, detail::AngleSpace::ASYMMETRIC>>>
		__stats1("angle.degree.asymmetric");

		namespace eckit::stats::util {


		#if defined(ECKIT_GEO_ECKIT_THREADS)

Feature/eckit stats #154

Are you sure you want to change the base?

Feature/eckit stats #154

Conversation

pmaciel commented Dec 9, 2024

pmaciel commented Dec 9, 2024

codecov-commenter commented Dec 9, 2024 • edited Loading

Codecov Report

simondsmart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 9, 2024 •

edited

Loading