Development

[tbertus]

Hey Connor,

These are the changes we spoke about sometime ago. Caching the results of the predictor step so the convolution does not need to be repeated during the corrector steps. Let me know what changes you want me to make if you decide to accept this.

Thanks,
Thomas

[cglosser]

Hey Thomas -

I want to noodle a little bit more on getting rid of the first_call variable since flags get really ugly really fast. In the next day or two, if you can, try to find a copy of Uncle Bob's Clean Code (it's a really great reference, too!) and see what he has to say about flags. I don't think it'll be too hard to refactor evaluate into evaluate_past and evaluate_present or similar (in which case the calls to evaluate(first_call = false) just become evaluate_present()), but since you did the work here read Clean Code, see if the arguments in Clean Code justify a little more work, and, ultimately, make things easier to understand. I'll defer to your professional opinion :)

Connor

[tbertus]

I'll give it a look!

[tbertus]

I removed the first_call flag and separated the evaluate functions into first_evaluation_of_timestep and evaluate. I gave the functions chapter of Clean Code a look and wonder if you think the first_evaluation_of_timestep function could be further broken up.

[cglosser]

One thing you can do if you're really feeling suave is to mention an issue number in your commit message. So, for instance, if you had said "Closes #42" in the commit where you got the Travis stuff working, Github would automatically link that together and close the issue. I think this is the coolest thing because if you're diligent about keeping issues around and mentioning them, it becomes super clear what you're trying to do (via the issues) and what commits address them (via the links).

[cglosser]

Shouldn't this be return results + past_terms_of_convolution? If evaluate is called, and then evaluate_present_field is called, results gets multiple "presents" added to it, no?

[tbertus]

results is set to zero at the top of evaluate_present_field so it has the contribution of a one "present" at a time. As a matter of curiosity, because the function returns a reference, would return results + past_terms_of_convolution work?

[cglosser]

Ah, yes, ok - that makes sense. There's still something I'm not seeing, though; ultimately I think evaluate is really just past_part + present_part. You've moved the logic for present_part into 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.

[tbertus]

I am not sure I follow. As of now, evaluate computes and stores the past and adds it to the present. evaluate_present_field zeros out results computes the updated present contribution and adds it to the past_part that was calculated by evaluate

[cglosser]

Hmm, so, in my head evaluate should always give you the full field (perhaps inefficiently) because it's the virtual function prescribed by InteractionBase. "An interaction can always be evaluated", 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_past and resultArray &AIM::DirectInteraction::evaluate_present, that each populate an internal array of known size (like results). Then you'll have something like this:

const InteractionBase::ResultArray &AIM::DirectInteraction::evaluate(const int time_idx) {
    evaluate_past(time_idx);
    evaluate_present(time_idx);
    results = past_results + present_results;
    return results;
}

and

const InteractionBase::ResultArray &AIM::DirectInteraction::evaluate_present(const int time_idx) {
    // Assumes evaluate_past has already been called for this time_idx
 
    // <Logic for present update>
    results = past_results + present_results;
    return results;
}

To do this, it may make sense to hoist evaluate_past and evaluate_present into InteractionBase as pure virtual functions. This would be the case if every Interaction that ever exists must define those functions to make sense (which is probably the case).

[tbertus]

I agree that would be a good way to set things up. But to be clear evaluate can currently be used independent of evaluate_present_field. The functionality of evaluate now is the same as it was originally with the added side effect of storing past_results.

[cglosser]

I really really hate that, e.g., direct_interaction needs to change to accommodate a new requirement in the predictor/corrector. It indicates things are too tightly coupled together. I'm sure there's a better way to push all of this into the integrator (probably with a strategy), but I'm not sure how to do it yet.

[cglosser]

Hmm, so, in my head evaluate should always give you the full field (perhaps inefficiently) because it's the virtual function prescribed by InteractionBase. "An interaction can always be evaluated", 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_past and resultArray &AIM::DirectInteraction::evaluate_present, that each populate an internal array of known size (like results). Then you'll have something like this:

const InteractionBase::ResultArray &AIM::DirectInteraction::evaluate(const int time_idx) {
    evaluate_past(time_idx);
    evaluate_present(time_idx);
    results = past_results + present_results;
    return results;
}

and

const InteractionBase::ResultArray &AIM::DirectInteraction::evaluate_present(const int time_idx) {
    // Assumes evaluate_past has already been called for this time_idx
 
    // <Logic for present update>
    results = past_results + present_results;
    return results;
}

To do this, it may make sense to hoist evaluate_past and evaluate_present into InteractionBase as pure virtual functions. This would be the case if every Interaction that ever exists must define those functions to make sense (which is probably the case).

[cglosser]

Please don't repeat code in comments; why do you need to set observers equal to temp_observers?

[cglosser]

You could declare and define simultaneously if you think it's cleaner. I think I split it up because there's a lot going on there, particularly with the ternary rotating-frame if statement.