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

[WIP] Observer Pattern for UpdateState #346

Draft
wants to merge 10 commits into
base: NonlinearEAS
Choose a base branch
from

Conversation

henrij22
Copy link
Collaborator

@henrij22 henrij22 commented Jan 13, 2025

This is to be merged in Nonlinear EAS PR

I ve now got working code with https://godbolt.org/z/b9cM5e714
I implemented Broadcasters next to Observers, so there are both implementations right now.

OLD

I don't know how to get this to work without a major overhaul of the observer thing.
The problem is that i want to notify like

      this->notify(NonLinearSolverMessages::CORRECTION_UPDATED, x, correction_);

So now the problem is that x and correction can have a lot of different types, Eigen Vector, Matrices, SparseVectors ...
For that i subclassed IOObservable like

template <typename MessageType, typename VectorType = Eigen::VectorX<double>,
          typename VectorType2 = Eigen::VectorX<double>>
class IObservable

that works now if

void IObservable<MT, VT1, VT2>::notify(MT message, const VT1& vec, const VT2& dx) {
  auto vectorOfObserversOfASpecificMessage = observers_[message];
  for (auto&& obs : vectorOfObserversOfASpecificMessage)
    obs->update(message, vec, dx);
}

has an observer that is also templated with two vectors

template <typename MT, typename VectorType = Eigen::VectorX<double>, typename VectorType2 = Eigen::VectorX<double>>
class IObserver

virtual void updateImpl([[maybe_unused]] MessageType message, [[maybe_unused]] const VectorType& vec,
                          [[maybe_unused]] const VectorType2& dx) {}

for example like

class NewtonRaphson : public IObservable<NonLinearSolverMessages, typename NLO::DerivativeType, typename NLO::ValueType>

but that means that every observer has to be templated like that because we need in IOObservable

  using ObserverVector = std::vector<std::shared_ptr<IObserver<MessageType, VectorType, VectorType2>>>;
  using ObserverMap    = std::map<MessageType, ObserverVector>;

which then means that every derived class of IOObserver somehow needs to be convertable to that.
Now i have a problem:

  auto nonLinearSolverObserver = std::make_shared<NonLinearSolverLogger>();
  nr->subscribeAll(nonLinearSolverObserver);  

doenst work because of

no known conversion from 'shared_ptr<_NonArray<NonLinearSolverLogger>>' to 'shared_ptr<IObserver<NonLinearSolverMessages, Matrix<double, -1, -1, 0, -1, -1>, Matrix<double, -1, 1, 0, -1, 1>>>' for 1st argument
[build]   void subscribeAll(std::shared_ptr<IObserver<MessageType, VectorType, VectorType2>> observer);

So i don't really know how to convert this because

  void subscribeAll(std::shared_ptr<IObserver<MessageType, VectorType, VectorType2>> observer);

takes a shared ptr to some observer with these template arguments.

Maybe you have a good idea @tarun-mitruka @rath3t

Maybe this can work out
#260

@henrij22 henrij22 self-assigned this Jan 13, 2025
@henrij22 henrij22 added the enhancement New request to enhance existing features label Jan 13, 2025
@henrij22 henrij22 marked this pull request as draft January 13, 2025 14:17
@tarun-mitruka tarun-mitruka force-pushed the NonlinearEAS branch 2 times, most recently from 4f1ca33 to 1729aba Compare January 14, 2025 07:39
tarun-mitruka and others added 5 commits January 16, 2025 16:03
but why?
test run?

test runs

test runs

this wont work
@henrij22 henrij22 force-pushed the feature/updateStateObserver branch from 39eb462 to b26883b Compare January 16, 2025 15:10
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 77.27273% with 20 lines in your changes missing coverage. Please review.

Project coverage is 81.29%. Comparing base (acb6da3) to head (9e839cb).

Files with missing lines Patch % Lines
ikarus/controlroutines/pathfollowing.inl 0.00% 10 Missing ⚠️
...olver/newtonraphsonwithscalarsubsidiaryfunction.hh 0.00% 8 Missing ⚠️
ikarus/utils/broadcaster/broadcaster.hh 75.00% 2 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff                @@
##           NonlinearEAS     #346      +/-   ##
================================================
+ Coverage         80.72%   81.29%   +0.56%     
================================================
  Files                70       68       -2     
  Lines              2309     2299      -10     
================================================
+ Hits               1864     1869       +5     
+ Misses              445      430      -15     
Flag Coverage Δ
tests 81.29% <77.27%> (+0.56%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@henrij22 henrij22 force-pushed the feature/updateStateObserver branch from acb86e8 to 0f12922 Compare January 16, 2025 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New request to enhance existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants