-
Notifications
You must be signed in to change notification settings - Fork 3
Development #44
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
base: development
Are you sure you want to change the base?
Development #44
Changes from 2 commits
0cf0263
52156fb
5c14ee1
0d44ffb
1357e27
33fae63
d967f2a
7d9ee0f
417f800
a7bfe54
91d6b8a
eda915d
32577f8
28c92a0
75dce9f
2cec272
6ba5f6f
9961c14
492cb9c
616d85e
102ceb1
7cbdc23
10c54d8
96ebe23
8ccb2f0
d331aff
42db27e
85d3b3f
b0af103
9fe8f2f
c23b752
f8777e0
7e5be99
afed5c6
17697fb
ca8168c
42948a6
e74e86d
66cc336
3bf248c
54c8771
47bbd65
6b9308a
4817ee2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,7 @@ class DirectInteraction final : public HistoryInteraction { | |
| const double); | ||
|
|
||
| const ResultArray &evaluate(const int) final; | ||
| const ResultArray &first_evaluation_of_timestep(const int) final; | ||
| const ResultArray &evaluate_present_field(const int) final; | ||
|
Comment on lines
16
to
+17
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really really hate that, e.g., |
||
|
|
||
| private: | ||
| int num_interactions; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be
return results + past_terms_of_convolution? Ifevaluateis called, and thenevaluate_present_fieldis called,resultsgets multiple "presents" added to it, no?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resultsis set to zero at the top ofevaluate_present_fieldso it has the contribution of a one "present" at a time. As a matter of curiosity, because the function returns a reference, wouldreturn results + past_terms_of_convolutionwork?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes, ok - that makes sense. There's still something I'm not seeing, though; ultimately I think
evaluateis really justpast_part+present_part. You've moved the logic forpresent_partinto the other function but you don't call it here so I'm not sure what your intention is.And you're exactly right: using
return results + past;returns what's called an "rvalue" (so a reference to it is invalid). Coding it that way would require changing the function signature.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I follow. As of now,
evaluatecomputes and stores the past and adds it to the present.evaluate_present_fieldzeros outresultscomputes the updated present contribution and adds it to thepast_partthat was calculated byevaluateUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, so, in my head
evaluateshould always give you the full field (perhaps inefficiently) because it's the virtual function prescribed byInteractionBase. "An interaction can always beevaluated", so to speak, and you may not need the complexity of divorcing the past update from the present update (if, say, you're not using something as sophisticated as the predictor/corrector). So I think a good design here would be to implement two methods,ResultArray &AIM::DirectInteraction::evaluate_pastandresultArray &AIM::DirectInteraction::evaluate_present, that each populate an internal array of known size (likeresults). Then you'll have something like this:and
To do this, it may make sense to hoist
evaluate_pastandevaluate_presentintoInteractionBaseas pure virtual functions. This would be the case if everyInteractionthat ever exists must define those functions to make sense (which is probably the case).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that would be a good way to set things up. But to be clear
evaluatecan currently be used independent ofevaluate_present_field. The functionality ofevaluatenow is the same as it was originally with the added side effect of storingpast_results.