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

matrices not updated in bilateral model #68

Closed
YoelPH opened this issue Jan 11, 2024 · 6 comments
Closed

matrices not updated in bilateral model #68

YoelPH opened this issue Jan 11, 2024 · 6 comments

Comments

@YoelPH
Copy link
Contributor

YoelPH commented Jan 11, 2024

While debugging my code I stumbled over an interesting problem which only arises when changing the modalities of a bilateral model.

Updating the modalities in any model should update the observation_matrix. This also works for the ipsilateral side, but not for the contralateral side. It seems as if callback for the contralateral side is not successful. I am not 100% sure whether this is due to the synchronization or if there is another problem.

I am working on a fix, but I wanted to notify you since you probably can detect the problem faster and produce a better fix.

Edit: my fast fix was quite simple. Since the problem is that the contralateral side does not trigger callbacks anymore, I am applying a different syncing function for the modalities:

def init_dict_sync2(
    this: AbstractLookupDict,
    other: AbstractLookupDict,
) -> None:
    """Add callback to ``this`` to sync with ``other``."""
    def sync():
        other.clear()
        other.update(this)

    this.trigger_callbacks.append(sync)

Thus we also need to change the way modalities are synced in the init_synchronization function:

       # Sync modalities
        if self.is_symmetric["modalities"]:
            init_dict_sync2(
                this=self.ipsi.modalities,
                other=self.contra.modalities,
            )

I am sure that you can come up with a more elegant solution, as I am struggling a bit to get a good overview of all the callbacks and syncing functions.

@rmnldwg rmnldwg changed the title observation_matrix not updated in bilateral model matrices not updated in bilateral model Jan 17, 2024
@rmnldwg
Copy link
Owner

rmnldwg commented Jan 17, 2024

I noticed the same issue occurs for the transition matrix.

Doesn't your solution create an infinite loop? Because when changing the ipsilateral modality, it triggers the contralateral callbacks, which, in turn, should trigger the ipsilateral callbacks. That's why I defined these update_without_trigger() methods.

Currently, I am thinking of an implementation similar to what I did with the transition_tensor for the Edge class that does not require us to delete these matrices. But it's a bit harder in these cases...

@YoelPH
Copy link
Contributor Author

YoelPH commented Jan 17, 2024

Interestingly, it does not create an infinite loop Seemingly, the change of the contralateral modalities does not trigger a second callback since only the ipsilateral should be triggering the callback. Which probably means, that when setting contralateral modalities, no callbacks are initialized and the ipsilateral side is not synced. I.e. we only have a one way syncing.

Generally I would welcome a syncing approach that is global as you implemented before.

@rmnldwg
Copy link
Owner

rmnldwg commented Jan 17, 2024

Alright, then I'll work on that now:

A two-way sync for the modalities via the callbacks and globally cached functions for the transition and observation matrices.

rmnldwg added a commit that referenced this issue Jan 17, 2024
The graph `Representation` class now has a method to compute a hash that
solely depends on the parametrization of the edges in the class. This
may be used to skip recomputing the transition matrix when that hash dis
not change.

Related: #68
rmnldwg added a commit that referenced this issue Jan 17, 2024
This cache decorator takes the first argument (arg0) that is passed to
the decorated function and checks the cache based on its (hash) value.
The decorated function then gets executed with the remaining arguments.

Related: #68
rmnldwg added a commit that referenced this issue Jan 17, 2024
rmnldwg added a commit that referenced this issue Jan 17, 2024
Like the transition matrix, the observation and diagnose matrix are now
computed by a globally cached function.

Related: #68
rmnldwg added a commit that referenced this issue Jan 17, 2024
The new way of globally caching the matrices required some refactoring
and simplifying.

Fixes: #68
@rmnldwg
Copy link
Owner

rmnldwg commented Jan 17, 2024

Ok, this was a bit trickier than I thought 😅

Because the diagnose_matrices depend on three things:

  1. the modalities
  2. the data
  3. and the T-category that was requested

If either changes, it should be recomputed. So, I had to compute a hash for all three quantities (ideally somewhat fast) and combine them into one hash to make sure you always get the correct diagnose matrix.

The changes are now in the dev branch. All tests (and some new ones) pass, but I still have the feeling that there could be something wrong with this new implementation.

So, it'd be great if you could double check that you can still reproduce your results. Thanks!

@YoelPH
Copy link
Contributor Author

YoelPH commented Jan 18, 2024

I'll go through it on Monday to double check that everything is fine. Thanks for the changes!

@YoelPH
Copy link
Contributor Author

YoelPH commented Jan 23, 2024

I tested the trinary and the binary model with bilateral sampling and risk calculations. The results were good --> seems like everything works as expected. The computation time is only minimally increased (might also just be some CPU fluctuations^^). Looks like a perfect fix!

@rmnldwg rmnldwg closed this as completed in ce7af81 Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants