Integration loop DR recording - #2062
Draft
excaliburtb wants to merge 4 commits into
Draft
Conversation
…the swig thisown override for some reason.
excaliburtb
marked this pull request as draft
March 4, 2026 21:17
| } | ||
|
|
||
| template<typename T, typename U> | ||
| void register_integgroup_wih_mm(T & grp, const std::string & nameIn) |
Contributor
There was a problem hiding this comment.
Suggested change
| void register_integgroup_wih_mm(T & grp, const std::string & nameIn) | |
| void register_integgroup_with_mm(T & grp, const std::string & nameIn) |
Comment on lines
+90
to
+91
| // DRBinary::set_job_class("integration"); | ||
| // DRBinary::register_group_with_mm(this, "Trick::IntegJobDRBinary"); |
Contributor
There was a problem hiding this comment.
Why are these commented out?
| @@ -0,0 +1,139 @@ | |||
| #include "trick/IntegrationJobDataRecordGroup.hh" | |||
Contributor
There was a problem hiding this comment.
source file should match header file name
Comment on lines
+6
to
+25
| int getStepsForIntegType(Integrator_type integ_type) | ||
| { | ||
| static int stepsPerType[User_Defined+1]{}; | ||
| if(stepsPerType[Euler] == 0) | ||
| { | ||
| stepsPerType[Euler] = 1; | ||
| stepsPerType[Euler_Cromer] = 1; | ||
| stepsPerType[Nystrom_Lear_2] = 2; | ||
| stepsPerType[Runge_Kutta_2] = 2; | ||
| stepsPerType[Modified_Midpoint_4] = 3; | ||
| stepsPerType[Runge_Kutta_4] = 4; | ||
| stepsPerType[Runge_Kutta_Gill_4] = 4; | ||
| stepsPerType[Runge_Kutta_Fehlberg_45] = 6; | ||
| stepsPerType[Runge_Kutta_Fehlberg_78] = 12; | ||
| stepsPerType[ABM_Method] = 4; | ||
| stepsPerType[User_Defined] = 1; | ||
| } | ||
|
|
||
| return stepsPerType[integ_type]; | ||
| } |
Contributor
There was a problem hiding this comment.
Is this function (and the template functions below) meant to be static/local-only?
Comment on lines
+108
to
+109
| // DRBinary::set_job_class("integration"); | ||
| // DRBinary::register_group_with_mm(this, "Trick::IntegJobDRBinary"); |
| ${TRICK_HOME}/include/trick/io_alloc.h \ | ||
| ${TRICK_HOME}/include/trick/message_proto.h \ | ||
| ${TRICK_HOME}/include/trick/message_type.h | ||
| object_${TRICK_HOST_CPU}/IntegJobDRGroup.o: IntegJobDRGroup.cpp ${TRICK_HOME}/include/trick/IntegrationJobDataRecordGroup.hh \ |
Contributor
There was a problem hiding this comment.
integJobDRGroup.o needs the hdf5 flags also in the Makefile
$(OBJ_DIR)/IntegJobDRGroup.o: TRICK_CXXFLAGS += -I$(HDF5)/include
$(OBJ_DIR)/io_IntegJobDRGroup.o: TRICK_CXXFLAGS += -I$(HDF5)/include
Comment on lines
+19
to
+47
| #ifdef SWIG | ||
| %feature("compactdefaultargs","0") ; | ||
| %feature("shadow") Trick::IntegJobDRBinary::IntegJobDRBinary(const std::string& in_name, Trick::IntegLoopScheduler& integSchedulerRefIn) %{ | ||
| def __init__(self, *args): | ||
| this = $action(*args) | ||
| try: self.this.append(this) | ||
| except: self.this = this | ||
| this.own(0) | ||
| self.this.own(0) | ||
| %} | ||
|
|
||
| %feature("shadow") Trick::IntegJobDRAscii::IntegJobDRAscii(const std::string& in_name, Trick::IntegLoopScheduler& integSchedulerRefIn) %{ | ||
| def __init__(self, *args): | ||
| this = $action(*args) | ||
| try: self.this.append(this) | ||
| except: self.this = this | ||
| this.own(0) | ||
| self.this.own(0) | ||
| %} | ||
|
|
||
| %feature("shadow") Trick::IntegJobDRHDF5::IntegJobDRHDF5(const std::string& in_name, Trick::IntegLoopScheduler& integSchedulerRefIn) %{ | ||
| def __init__(self, *args): | ||
| this = $action(*args) | ||
| try: self.this.append(this) | ||
| except: self.this = this | ||
| this.own(0) | ||
| self.this.own(0) | ||
| %} | ||
| #endif |
Contributor
There was a problem hiding this comment.
should be blocked in // clang-format off/on guards
Comment on lines
+90
to
+92
| #ifdef SWIG | ||
| %feature("compactdefaultargs","1") ; | ||
| #endif |
Contributor
There was a problem hiding this comment.
block in // clang-format off/on guard
| virtual int init(bool is_restart = false) override; | ||
| virtual int data_record(double in_time) override; | ||
|
|
||
| Trick::IntegLoopScheduler* integSchedulerPtr { }; |
Contributor
There was a problem hiding this comment.
does this (and the other 2) need trick_io comment?
Comment on lines
+6
to
+25
| int getStepsForIntegType(Integrator_type integ_type) | ||
| { | ||
| static int stepsPerType[User_Defined+1]{}; | ||
| if(stepsPerType[Euler] == 0) | ||
| { | ||
| stepsPerType[Euler] = 1; | ||
| stepsPerType[Euler_Cromer] = 1; | ||
| stepsPerType[Nystrom_Lear_2] = 2; | ||
| stepsPerType[Runge_Kutta_2] = 2; | ||
| stepsPerType[Modified_Midpoint_4] = 3; | ||
| stepsPerType[Runge_Kutta_4] = 4; | ||
| stepsPerType[Runge_Kutta_Gill_4] = 4; | ||
| stepsPerType[Runge_Kutta_Fehlberg_45] = 6; | ||
| stepsPerType[Runge_Kutta_Fehlberg_78] = 12; | ||
| stepsPerType[ABM_Method] = 4; | ||
| stepsPerType[User_Defined] = 1; | ||
| } | ||
|
|
||
| return stepsPerType[integ_type]; | ||
| } |
Contributor
There was a problem hiding this comment.
static constexpr array instead?
Suggested change
| int getStepsForIntegType(Integrator_type integ_type) | |
| { | |
| static int stepsPerType[User_Defined+1]{}; | |
| if(stepsPerType[Euler] == 0) | |
| { | |
| stepsPerType[Euler] = 1; | |
| stepsPerType[Euler_Cromer] = 1; | |
| stepsPerType[Nystrom_Lear_2] = 2; | |
| stepsPerType[Runge_Kutta_2] = 2; | |
| stepsPerType[Modified_Midpoint_4] = 3; | |
| stepsPerType[Runge_Kutta_4] = 4; | |
| stepsPerType[Runge_Kutta_Gill_4] = 4; | |
| stepsPerType[Runge_Kutta_Fehlberg_45] = 6; | |
| stepsPerType[Runge_Kutta_Fehlberg_78] = 12; | |
| stepsPerType[ABM_Method] = 4; | |
| stepsPerType[User_Defined] = 1; | |
| } | |
| return stepsPerType[integ_type]; | |
| } | |
| int getStepsForIntegType(Integrator_type integ_type) | |
| { | |
| static constexpr int stepsPerType[User_Defined + 1] = { | |
| 1, // Euler | |
| 1, // Euler_Cromer | |
| 2, // Nystrom_Lear_2 | |
| 2, // Runge_Kutta_2 | |
| 3, // Modified_Midpoint_4 | |
| 4, // Runge_Kutta_4 | |
| 4, // Runge_Kutta_Gill_4 | |
| 6, // Runge_Kutta_Fehlberg_45 | |
| 12, // Runge_Kutta_Fehlberg_78 | |
| 4, // ABM_Method | |
| 1 // User_Defined | |
| }; | |
| return stepsPerType[integ_type]; | |
| } |
Contributor
There was a problem hiding this comment.
my suggestion isn't any better. it fixes the potential race condition but adds ordering dependent behaviour that would need to be maintained alongside any changes to integrator interface
should really just be a switch tree with the function itself made static/constexpr
Contributor
|
and your formatting lol |
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
2 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.
primarily useful for debugging, we do find ourselves needing to log the intermediate steps of a multi-step integration technique (like RK4) sometimes. we usually resort to prints but this class which just extends the current DR* ones could help.