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

Displayin the mean sound speed in the database tab #95

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

eriffon
Copy link
Contributor

@eriffon eriffon commented Dec 4, 2024

We have a requirement to quickly access the mean sound speed (weighted harmonic mean) of a cast without having to explicitly call the profile statistics calculation in SSM. A new column in the table of the Database tab would fulfill this requirement. This PR is a prototype solution. However, the weighted harmonic mean calculation is refreshed at every table update, which is far from optimal. A better solution would be to store the mean sound speed when the cast gets read into SSM and is permanently stored in the profile.Metadata class (or some other member of the Profile class). That would probably require a change to the SQlite3 data structure, wouldn't it? I presume that this would break backwards compatibility? I just wanted to hear some thoughts on this. Thanks.

@giumas
Copy link
Collaborator

giumas commented Jan 13, 2025

@eriffon, I would avoid to store in the database a value that can be recalculated from the samples. The main risk is that the stored value gets outdated in one of the many ways that the profile can be edited.

An alternative to explore would be to have the weighted harmonic mean also as a "cached property": https://docs.python.org/3/library/functools.html#functools.cached_property
This way the database tab may use such a value knowing the risk of returning an outdated value. Maybe we can also try to add a mechanism that always invalidate cached properties when a profile is edited.

@eriffon
Copy link
Contributor Author

eriffon commented Jan 14, 2025

@giumas, thanks for your thoughts and your suggestion to use a "cached property". I will look into this and modify the PR accordingly to get a prototype working.

@eriffon
Copy link
Contributor Author

eriffon commented Jan 23, 2025

@giumas, I've explored the use of a "cached property". It seems promising and I believe that a mechanism that always invalidates cached properties when a profile is edited would be simple enough to develop. Right now, I think that whenever the current profile is stored in the project db soundspeed.store_data() would be a good time to update the attribute.

My concern is more when the user switches from one project to the other. The "cached_property" is only valid as long as the profile.proc_mean_speed attribute instance exists. Switching projects causes the attribute instance to vanish and triggers a recalculation each time. With projects that contain hundreds of profiles, the response time is noticeable slower. Is this a fair description of what is happening? Any suggestion on how the attribute instance could be retained when switching between projects? I don't really see how except by storing the value in the database.

@giumas
Copy link
Collaborator

giumas commented Jan 26, 2025

@eriffon, this is definitely a though decision. Would you be willing to give a try on adding the value to the database, together with a proposal on how to keep it updated in case of editing?

@eriffon
Copy link
Contributor Author

eriffon commented Jan 27, 2025

@giumas, yes. As this is an important request for us, I'm willing to give it a try.

Another thought that crossed my mind was to have SoundSpeedManager write a temporary file in the user's profile whenever the user switches projects. This file would contain the results of the last mean sound speed calculation. It would be flushed whenever SoundSpeedManager is closed. This would avoid having to modify the setup.db while probably keeping the GUI response times adequate.

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

Successfully merging this pull request may close these issues.

2 participants