Reworking of the ObserverTimeEvolution #511

JanNiklasB · 2024-12-19T14:45:05Z

Dear All,

in this pull request, I want to introduce a reworked ObserverTimeEvolution.
I replaced the addTimeRange function which filled the detList array with a function
that takes the required index as an argument and returns the time that should be observed.
For that, I also changed other parts of the ObserverTimeEvolution class to
correctly utilize that new getTime function.
I also kept the option of using a detList by utilizing a new constructor.
Furthermore, it is possible to use a user-defined getTime function by utilizing
the setUserDefinedGetTime function.

Additionally, I fixed the issue mentioned in #510.
For that, it was necessary to change some tests, since those handled
the maximum observed time like max = numb * dist when it should
be handled like max = min + numb * dist.

-> detList still available as option -> custom function option added

…d getTime function

doc/pages/example_notebooks/Diffusion/MomentumDiffusion.ipynb

test/testBreakCondition.cpp

include/crpropa/module/Observer.h

- fixed wrong scaling in constructor - fixed unexpected behavior in getTimes - added more detailed documentation - replaced customGetTime with virtual on getTime since it is only used inside c++

- modified Test for checkDetection - added Test for behavior with arrays instead of getTime function - added clear and empty function

- further fixed a floating point issue in testBreakCondition

JanNiklasB · 2025-03-06T14:31:10Z

After discussing pull request with @JulienDoerner, I added setter and getter functions as well as some older functions that I previously removed to preserve compatibility.

The ObserverTimeEvolution now creates the protected vector detList when calling any functions that require detList like addTime or addTimeRange.
Futhermore, function to edit detList more freely like clear and setTimes were added.
Since it is only necessary to use custom functions inside of c++, the override of the getTime can now be done directly by overriding the virtual function getTime.
I added tests to testBreakCondition where the behavior of the arrays is tested in more detail.

For a more detailed description of the single changes, see the comments.

JanNiklasB · 2025-03-06T14:33:43Z

include/crpropa/module/Observer.h

@@ -240,52 +240,119 @@ class ObserverParticleIdVeto: public ObserverFeature {
 This observer is very useful if the time evolution of the particle density is needed. It detects all candidates in lin-spaced, log-spaced, or user-defined time intervals and limits the nextStep of candidates to prevent overshooting of detection intervals.
 */
 class ObserverTimeEvolution: public ObserverFeature {
-private:
+protected:


changed private to protected, so the variables can be accessed in derived classes

JanNiklasB · 2025-03-06T14:34:39Z

include/crpropa/module/Observer.h

+	 Use this function to create a detList with can then be modified.
+	 When detList is not empty its entries are used instead of a runtime calculation of the times.	 
+	 */
+	void constructDetListIfEmpty();


this function enables the user to manually construct detList

JanNiklasB · 2025-03-06T14:36:29Z

src/module/Observer.cpp

@@ -246,7 +246,7 @@ std::string ObserverParticleIdVeto::getDescription() const {
 ObserverTimeEvolution::ObserverTimeEvolution() {}

 ObserverTimeEvolution::ObserverTimeEvolution(double min, double dist, double numb) {
-	this->max = min + numb * dist;
+	this->max = min + (numb - 1) * dist;


fixes the original bug, where the last time to observe was too large because of this

JanNiklasB · 2025-03-06T14:41:01Z

test/testBreakCondition.cpp

-  times = {1, 1000};
+  times.clear();
+	for (int i=0; i<4; i++)
+		times.push_back(pow(1000., i / 3.));


pow(1000, 2/3) does not exactly return 100 which caused a wrong negative in line 345

maybe you can check for EXPECT_NEAR with some small errorband.

JanNiklasB · 2025-03-06T14:44:27Z

test/testBreakCondition.cpp

  EXPECT_TRUE(c.hasProperty("Detected"));
 }

+TEST(ObserverFeature, TimeEvolutionArray) {


This test checks if detList is created and used correctly when using the modifying functions.

JulienDoerner

Hey @JanNiklasB,

the overall changes look good for me and it improves the usability for non-uniform observations.
I left some comments about code style (see below).

And can you please add your changes to the CHANGELOG.md.

JulienDoerner · 2025-03-06T15:47:35Z

include/crpropa/module/Observer.h

@@ -240,8 +240,22 @@ class ObserverParticleIdVeto: public ObserverFeature {
 This observer is very useful if the time evolution of the particle density is needed. It detects all candidates in lin-spaced, log-spaced, or user-defined time intervals and limits the nextStep of candidates to prevent overshooting of detection intervals.
 */
 class ObserverTimeEvolution: public ObserverFeature {
-private:
+protected:
+	int numb;


In our coding style all variables should be given with full names. Therefore you should use number or nObservations instead. Also add an inline comment what this value us.

JulienDoerner · 2025-03-06T15:51:05Z

include/crpropa/module/Observer.h

+	virtual double getTime(std::size_t index) const;
+	double getMin() const {return min;}
+	double getMax() const {return max;}
+	int getNumb() const {return numb;}


Use full names for functions

JulienDoerner · 2025-03-06T16:00:49Z

src/module/Observer.cpp

 }

 void ObserverTimeEvolution::addTimeRange(double min, double max, double numb, bool log) {
 	for (size_t i = 0; i < numb; i++) {
-		if (log == true) {
+		if (log) {
+			if ( min == 0 ){


Is it better to prove for min <= 0?

JulienDoerner · 2025-03-06T16:04:05Z

src/module/Observer.cpp

+	if (!detList.empty()) {
+		return detList.at(index);
+	} else if (islog) {
+		if ( min == 0 ){


Is it necessary to prove this in every getTime call, which is called in every propagation step? I guess a proof only in the setter functions would be enough.

right, if it is set right in the setter functions the check here would be unnecessary

JulienDoerner · 2025-03-06T16:06:51Z

test/testBreakCondition.cpp

@@ -273,37 +277,91 @@ TEST(ObserverFeature, TimeEvolutionLog) {
  obs.setFlag("Detected", "Detected");
  // usage of a log scaling for the observer
  bool log = true;
-  obs.add(new ObserverTimeEvolution(5, 5, 2, log));
+  obs.add(new ObserverTimeEvolution(5, 10.05, 2, log));


Does this test really reflect the log-scaling? If it only has two points it would be the same in linear or log

right, it should at least test 3 points

JulienDoerner · 2025-03-06T16:10:18Z

test/testBreakCondition.cpp

-  times = {1, 1000};
+  times.clear();
+	for (int i=0; i<4; i++)
+		times.push_back(pow(1000., i / 3.));


maybe you can check for EXPECT_NEAR with some small errorband.

- Replaced EXPECT_TRUE with EXPECT_NEAR in TimeEvolutionArray

- all changes to variables now over setter functions - valid range check for min if isLogarithmicScaling = true now in setMinimum

Jan-Niklas Bohnensack and others added 7 commits July 31, 2024 15:47

Replaced detList with getTime

37cde69

-> detList still available as option -> custom function option added

Modified ObserverTimeEvolution to include option to use a user define…

05134f1

…d getTime function

Removed not used variable value

18eb0a5

maxD was chosen too small, produced error later in code

5cfc6bf

Merge branch 'CRPropa:master' into OMP-Critical-Issue

0896f73

fixed scaling issue and added description to constructor

a46d27b

testObserverTimeEvolution expected wrong point of detection (fixed)

f0f3d24

JanNiklasB commented Dec 19, 2024

View reviewed changes

doc/pages/example_notebooks/Diffusion/MomentumDiffusion.ipynb Outdated Show resolved Hide resolved

JanNiklasB commented Dec 19, 2024

View reviewed changes

test/testBreakCondition.cpp Outdated Show resolved Hide resolved

JanNiklasB commented Dec 19, 2024

View reviewed changes

include/crpropa/module/Observer.h Show resolved Hide resolved

JanNiklasB commented Dec 19, 2024

View reviewed changes

include/crpropa/module/Observer.h Outdated Show resolved Hide resolved

lukasmerten self-assigned this Jan 9, 2025

Jan-Niklas Bohnensack added 4 commits March 4, 2025 01:08

Fixed multiple bugs and added setter/getter functions

86f9891

- fixed wrong scaling in constructor - fixed unexpected behavior in getTimes - added more detailed documentation - replaced customGetTime with virtual on getTime since it is only used inside c++

Added Tests and and added some features to ObserverTimeEvolution

09c8f99

- modified Test for checkDetection - added Test for behavior with arrays instead of getTime function - added clear and empty function

Improved handling of vector capacities

78c4cb3

- further fixed a floating point issue in testBreakCondition

reversed a pevious fix which is now fixed in Observer

a7325db

JanNiklasB commented Mar 6, 2025

View reviewed changes

JulienDoerner requested changes Mar 6, 2025

View reviewed changes

Jan-Niklas Bohnensack added 3 commits March 6, 2025 18:48

Changed TimeEvolutionLog test so 3 values are tested

09c749d

- Replaced EXPECT_TRUE with EXPECT_NEAR in TimeEvolutionArray

changed short names to full names

3fc45cf

- all changes to variables now over setter functions - valid range check for min if isLogarithmicScaling = true now in setMinimum

added changes to changelog

8ce85f1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reworking of the ObserverTimeEvolution #511

Reworking of the ObserverTimeEvolution #511

JanNiklasB commented Dec 19, 2024 •

edited

Loading

JanNiklasB commented Mar 6, 2025

JanNiklasB Mar 6, 2025

JanNiklasB Mar 6, 2025

JanNiklasB Mar 6, 2025

JanNiklasB Mar 6, 2025

JulienDoerner Mar 6, 2025

JanNiklasB Mar 6, 2025

JulienDoerner left a comment •

edited

Loading

JulienDoerner Mar 6, 2025

JulienDoerner Mar 6, 2025

JulienDoerner Mar 6, 2025

JulienDoerner Mar 6, 2025

JanNiklasB Mar 6, 2025

JulienDoerner Mar 6, 2025

JanNiklasB Mar 6, 2025

JulienDoerner Mar 6, 2025

Reworking of the ObserverTimeEvolution #511

Are you sure you want to change the base?

Reworking of the ObserverTimeEvolution #511

Conversation

JanNiklasB commented Dec 19, 2024 • edited Loading

JanNiklasB commented Mar 6, 2025

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

JulienDoerner left a comment • edited Loading

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

JanNiklasB commented Dec 19, 2024 •

edited

Loading

JulienDoerner left a comment •

edited

Loading