Yuri-SVB / Great_Wall

Protocol and application for providing Kerckhoffian, 0-trust, deviceless coercion-resistance in self-custody.
MIT License
24 stars 14 forks source link

Reuse fractal formula iterations #66

Closed flatFeather closed 7 months ago

flatFeather commented 7 months ago

Reuse fractal formula iterations. Issue #30

MuhammadMuradG commented 7 months ago

Hi, @flatFeather! This PR seems to be a duplication of #61, is this correct or there is a difference?

flatFeather commented 7 months ago

Hi @MuhammadMuradG, It is correct. Unfortunately it seems that someone and I have been working on the same issue. I think I told you that I was going to be with this issue. It is very difficult for me to continually check who is with what, we should find a way to centralize that.

flatFeather commented 7 months ago

Same approach taked:

flatFeather commented 7 months ago

After some manual performance tests: It takes 15 seconds to go from one derivation level to another, generating the images. After this code it takes only 5 seconds.

MuhammadMuradG commented 7 months ago

Hi @flatFeather!

You can simply if you intend to work on specific issue create a PR that describe what you intend to do before even create any valuable commits then convert it to draft which is mean the work in progress... However, the contributor of PR 60 did not address all the issue raised and it seems that he will not continue work on his PR.

Also, you wasn't need to create multi consecutive comments to describe your PR instead you can edit PR description box.

MuhammadMuradG commented 7 months ago

It seems you are on the right way, I will left some point you need to address inline.

MuhammadMuradG commented 7 months ago

I gave it a new shot of review, and my previous suggestion about "resetting the attribute" will introduce a bug relating to the fact that this suggestion discarding the different between p_params being used and then I realized that we need to move z = c code line before if statement but after making this change I can not find any performance enhancement, so, how you tested the enhancement performance?

flatFeather commented 7 months ago

Here how I tested the enhancement performance:

https://github.com/Yuri-SVB/Great_Wall/assets/50509521/25a8d87e-874f-423b-9da9-51d642f779d2

Yuri-SVB commented 7 months ago

Muhammad seems to be right: the last change seemed to change nothing.

flatFeather commented 7 months ago

If the value of _pparam is updated in the update() method during the app flow and you want that change to be reflected in the fractal calculation, then you will need to recalculate all the points in the set each time _pparam is updated. This means that you cannot use the cache to store the previous results, since you need to recalculate the values with the new _pparam.

flatFeather commented 7 months ago

I have modified the code so that when using the cache it takes into account the _pparam to be used or not. But the use case is very corner case, I could not even reproduce this case to see the time difference. There is no difference in the normal flow of the app.

MuhammadMuradG commented 7 months ago

Since this PR doesn't introduce any enhancements, is there still a need to merge it? However, I think you may have been able to conclude that this approach wouldn't yield significant improvements if you had think in the potential benefits beforehand.