Skip to content
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

Clean up observer interfaces #1087

Merged
merged 2 commits into from
Nov 7, 2024
Merged

Clean up observer interfaces #1087

merged 2 commits into from
Nov 7, 2024

Conversation

bartgol
Copy link
Collaborator

@bartgol bartgol commented Nov 5, 2024

Observers have lots of interfaces, depending on how they are called (e.g., with/without xdot/xdotdot, with Vector's vs a MultiVector, etc). Rather than having lots of XYZimpl functions, try to make all the interfaces call a single impl function. This makes modifications of actual implementation easier, since we only have to mod one fcn.

As a side note: I think I would remove all the MultiVector interfaces in the discretization: as I did in one of the observers, the interface that takes a MultiVector (consisting of x, possibly xdot, possibly xdotdot) can simply extract the MV columns, and call the proper interface with Vector's. This would reduce the amount of interfaces we need to implement in our classes. But that's just a wishlist thought.

@bartgol bartgol requested a review from mperego November 5, 2024 16:35
@bartgol bartgol self-assigned this Nov 5, 2024
Copy link
Collaborator

@mperego mperego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested it but it looks good to me. Thanks for cleaning this up!

Would you mind adding a brief description in the header files about how these observers are used?

std::size_t value_width = precision + 7;
*out << std::scientific << std::showpoint << std::setprecision(precision) << std::left;

// Note that we don't have g_names support in thyra yet. Once
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be nice.

}
}
*out << "\n";
if (not firstResponseObtained and calculateRelativeResponses) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are these relative responses?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really know. I just re-arranged/fixed existing code here...

@bartgol bartgol merged commit e1e9567 into master Nov 7, 2024
@bartgol bartgol deleted the bartgol/observers-cleanup branch November 7, 2024 02:55
@bartgol
Copy link
Collaborator Author

bartgol commented Nov 7, 2024

I merged so I can take care of any failure we may get tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants