search

lab-cosmo / chemiscope

An interactive structure/property explorer for materials and molecules
http://chemiscope.org
BSD 3-Clause "New" or "Revised" License
119 stars 29 forks source link

Added a color bar #312

Closed MaximeScope closed 7 months ago

MaximeScope commented 8 months ago

Implemented a color bar to scale atom colors based on their properties.

ceriottm commented 8 months ago

looks quite nice. you and @Luthaf should check and merge #313 before doing anything here just to make sure there are no further issues with multiple panels and colors.

re the color bar I think it'd look better as a horizontal scale, that fits nicely on the top right, extending up until the option buttons. I think it'd also be better to just dump the default 3dmol color scale and replicate the plotly ones, so there is a 1:1 mapping between the map and the structure panels.

ceriottm commented 8 months ago

So, a few suggestions to clean up the code:

  1. I'd try and either make a class, or consolidate the functions; I see no point in having a dozen helper functions each of which is called only once
  2. To avoid ambiguity with the colors, I'd use only custom colors, and do them based on the plotly palettes. the palettes available in the map and in the viewer should be matched - they are defined in map/colorscales.ts and perhaps could be moved to a place where they can be easily accessed by both the map and the structure panel
  3. I'd define the gradient with an array of color strings rather than hardcoding 5 steps
  4. I'd make the horizontal size of the colorbar self-adjusting depending on the size of the window, makin it reach close to the control buttons
ceriottm commented 8 months ago

Nice. One bug though: it doesn't rescale existing colorbars when you add new panels (for some reasons however it does when you remove panels so it looks like you forgot some hooks. Has there been work on points 1-3? I think that unifying the color schemes between the two panels is quite important, and given that this might change the names we should do it before there are datasets out there that use this functionality.

Luthaf commented 8 months ago

I think we can keep points 1-3 for a future PR! (one cleanup PR for 1 and one merging the palettes for 2-3)

MaximeScope commented 8 months ago

Nice. One bug though: it doesn't rescale existing colorbars when you add new panels (for some reasons however it does when you remove panels so it looks like you forgot some hooks. Has there been work on points 1-3? I think that unifying the color schemes between the two panels is quite important, and given that this might change the names we should do it before there are datasets out there that use this functionality.

Good catch! I forgot to call my function when duplicating a viewer (in the grid.ts file). So, I've just pushed this small fix.

MaximeScope commented 8 months ago

So, I might still need to clean up the code a little bit but at least, the color palettes are now the same between the map and the viewer! However, I came across a strange problem which seems to involve multiple loading of the same viewer (some functions may be called several times instead of once)... Although this does not really affect the user, it could potentially slow down the code as well as Chemiscope's development. For example, I had to state that the properties couldn't be infinite (and this issue wasn't so straightforward to spot). However, if the viewer was loading properly, I shouldn't need to state that condition when using my colorFunction. So far, I haven't been able to work out where this potential problem could be coming from.

ceriottm commented 8 months ago

Thanks a lot. It's much much better to have the same set of palettes, and it'll be also much easier to add more, now that there's a single point to edit them.

So, I might still need to clean up the code a little bit but at least, the color palettes are now the same between the map and the viewer! However, I came across a strange problem which seems to involve multiple loading of the same viewer (some functions may be called several times instead of once)... Although this does not really affect the user, it could potentially slow down the code as well as Chemiscope's development. For example, I had to state that the properties couldn't be infinite (and this issue wasn't so straightforward to spot). However, if the viewer was loading properly, I shouldn't need to state that condition when using my colorFunction. So far, I haven't been able to work out where this potential problem could be coming from.

I think this has to do with the property loading and setting. Might be a side-effect of a change I did a while ago because the properties had to be loaded before creating the map. I don't understand fully the property loading and the sync to the HTML option modals, so I think that this is for @Luthaf to look into. In all events, it's a separate issue so as far as I am concerned this could be merged.

Luthaf commented 8 months ago

For example, I had to state that the properties couldn't be infinite (and this issue wasn't so straightforward to spot). However, if the viewer was loading properly, I shouldn't need to state that condition when using my colorFunction. So far, I haven't been able to work out where this potential problem could be coming from.

I don't understand what you are taking about. What do you mean with "I had to state that the properties couldn't be infinite"? The properties values should be able to be infinite/NaN even if that might reflect some issues with the underlying dataset.

Could you give a set of steps that lead to the issue you are referring to and what's the issue in question?

ceriottm commented 8 months ago

For example, I had to state that the properties couldn't be infinite (and this issue wasn't so straightforward to spot). However, if the viewer was loading properly, I shouldn't need to state that condition when using my colorFunction. So far, I haven't been able to work out where this potential problem could be coming from.

I don't understand what you are taking about. What do you mean with "I had to state that the properties couldn't be infinite"? The properties values should be able to be infinite/NaN even if that might reflect some issues with the underlying dataset.

Could you give a set of steps that lead to the issue you are referring to and what's the issue in question?

I think that he refers to the fact that when chemiscope is loaded that function gets called before the properties are initialized and so the extremes are nans. tracking what happens when chemiscope is loaded is a bit of a pain because of all the asynchronous code, but I also had the impression that the whole widget is initialized, and then the settings are applied, which might be making the loading, particularly for large structures, much slower.

ceriottm commented 7 months ago

OK I had lost track, but it looks like this has been polished and is ready to merge. If there are still outstanding problems we can always fix them in a new PR.