simularium / simularium-website

Front end website for the Simularium project, includes the Simularium viewer
https://simularium.allencell.org
Apache License 2.0
7 stars 3 forks source link

color sessions: add function to compare uidisplaydata agent tree structures #588

Closed interim17 closed 1 month ago

interim17 commented 1 month ago

Time estimate or Size

small, mostly tests

Problem

Closes #581, advances #511

When retrieving stored color settings from browser storage, we will retrieve data according to trajectory name. It's possible for incompatible trajectory data structures to have the same name, so we need a utility to compare the agents and displayStates to ensure that the retrieved settings can be safely applied to the trajectory that is being loaded.

This PR is just the utility and unit tests, not an implementation of retrieving settings.

github-actions[bot] commented 1 month ago

Coverage report

St.:grey_question:
Category Percentage Covered / Total
🟑 Statements
66.73% (+0.49% πŸ”Ό)
690/1034
🟑 Branches
66.67% (+1.36% πŸ”Ό)
102/153
πŸ”΄ Functions
35.25% (+0.25% πŸ”Ό)
92/261
🟑 Lines
65.07% (+0.45% πŸ”Ό)
613/942

Test suite run success

129 tests passing in 7 suites.

Report generated by πŸ§ͺjest coverage report action from 3cd159c6e057baa8ab952d066d7f9d47dfb4043c

interim17 commented 1 month ago

LGTM.

Is it smelly for this to exist? Seems a little curious, but I'd need a better understanding of how UIDisplayData gets used to really comment on it.

I'm not very familiar with this use of the word smelly.

Is it smelly because it doesn't get used yet, so the purpose isn't clear? Or smelly because you don't know what if UIDisplayData is a good data structure for this use case?

For the latter question, I'll just say that UIDisplayData is a tree of agents and displayStates with associated color assignments. The viewer generates a tree of that type when parsing a trajectory file and passes it to the website to use, the website can either mutate it or generate new trees when changing color assignments.

@meganrm and I paired on this issue a while back and landed on UIDisplayData as a good existing data structure to store a snapshot of the current state of color assignments for a trajectory. Happy to give more context, and definitely open to input/feedback if you see potential issues here.

tyler-foster commented 1 month ago

I'm not very familiar with this use of the word smelly.

As in, is this a "code smell"?

Is it smelly because it doesn't get used yet, so the purpose isn't clear? Or smelly because you don't know what if UIDisplayData is a good data structure for this use case?

For the latter question, I'll just say that UIDisplayData is a tree of agents and displayStates with associated color assignments. The viewer generates a tree of that type when parsing a trajectory file and passes it to the website to use, the website can either mutate it or generate new trees when changing color assignments.

@meganrm and I paired on this issue a while back and landed on UIDisplayData as a good existing data structure to store a snapshot of the current state of color assignments for a trajectory. Happy to give more context, and definitely open to input/feedback if you see potential issues here.

Yessir, I meant the latter question. My concern stems from my lack of understanding of this part of your PR description:

It's possible for incompatible trajectory data structures to have the same name, so we need a utility

It sounds like we've arrived at a namespacing issue that now begets a special solution, rather than a fix to the namespacing issue. I'd have to have a better understanding of what it means in practice for the trajectory data structures to have the same name before I could provide an alternative idea. You guys could be totally correct in how you've solved this problem though. I say just keep moving forward with this solution and if it ends up feeling awkward for some reason you can always double back and change it again.

interim17 commented 1 month ago

It sounds like we've arrived at a namespacing issue that now begets a special solution, rather than a fix to the namespacing issue. I'd have to have a better understanding of what it means in practice for the trajectory data structures to have the same name before I could provide an alternative idea. You guys could be totally correct in how you've solved this problem though. I say just keep moving forward with this solution and if it ends up feeling awkward for some reason you can always double back and change it again.

Ok I see where you're at.

When we store color settings in browser storage the key we use is the filename. Say its "test". If we have {test: settings} stashed, and load a different trajectory, but also named "test", trying to apply the stashed settings will cause an error. This is an edge case, because names are likely to be unique, and this storage is ephemeral.

I agree I think its ok to go forward for now, so I'm going to merge, but if you have suggestions on a better way to handle the namespace in the browser storage, I'm open to it. Ultimately this is a convenience for temporarily storing/retrieving ongoing adjustments to colors and the more permanent feature, down the line, will be writing changes to a new trajectory file.