Add possibility to change the display mode (atom <-> structure)

[sofiia-chorna]

Where to play around with modes:

npm i
npm run start

Open the Chemical Shieldings example

tox -e docs
python3 -m http.server

Open examples/2-structure_map.html

TODO:

• change in EnvironmentInfo
• change in map
• change in structure
• move mode out of indexer
• add loader
• change UI of toggle
• testing (played with the examples from chemiscope.org & in doc/examples)

After review:

• UI: add titles to buttons; substruct height of buttons in the map; reset color range
• Code: document, delete usage of settings while changing the mode; fully recreate mapOptions
• Display errors as errors instread of warning --> throw error, change logging in separate PR
• Add mode to the settings

Note for exporting/importing of the dataset:

• If mode is changed and z-axes is not selected, we export NaN to JSON as z.min and z.max; hence it breaks once someone wants to import this dataset to the chemiscope. To do the import, it is necessary to delete the z-axes from the json settings (or set 0).
• This bug to be fixed in the separate PR

[Luthaf]

This is working very well! The one small bug I found was that the color bar range is not reset when switching mode.

I've updated the example dataset for CSD to include a read structure PCA (the previous data only contained a single PCA dimension, with the other two being 0 everywhere); you can get the updated version by removing app/example and starting the dev server again.

[Luthaf]

why is this needed? It it better to not use ! as much as possible, since it can hide errors that TS would have caught.

[sofiia-chorna]

I used ! it as _structure is initialised not directly in the constructor but in the function called in constructor -> TS complains that _structure might be undefined

[ceriottm]

had a quick look (will try to break it some more) - I know you've been talking about this already, but I noticed that it looks like settings for the structure viewers are preserved when you switch mode (e.g. if you set atoms to space filling, they remain space filling in both struc and env mode) while the map is reset. I find this very confusing.
Other quick comment: I think Environment is better than Atoms to indicate the local view mode.

[sofiia-chorna]

had a quick look (will try to break it some more) - I know you've been talking about this already, but I noticed that it looks like settings for the structure viewers are preserved when you switch mode (e.g. if you set atoms to space filling, they remain space filling in both struc and env mode) while the map is reset. I find this very confusing. Other quick comment: I think Environment is better than Atoms to indicate the local view mode.

@ceriottm
Thanks a lot for the feedback! Indeed, I could reset all settings of structure viewer too, if now it looks confusing, and rename Atoms to Environments

[Luthaf]

The code looks good! I like that you moved a bunch of functionalities to async, this will be nicer long term.

I'll try to break it a bit, and when @ceriottm is also done with trying to break the code we can merge this.

[Luthaf]

instead of isPerAtom, why not pass the mode around here? (same in the function below)

[ceriottm]

I tweaked the style a bit, and removed the title that seemed heavy and out of style with the rest of the UI.
I noticed a strange issue - if you fire up the website, then load the CS example and then switch back to say arginine, the space occupied by the toggler is not claimed back by the map, even though the div is removed.

Also, once we're done with the polish and the stress-testing, I think it'd be worth adding a sentence or two to the docs somewhere mentioning one can add both types of data to a json, and switch between the two.

[sofiia-chorna]

I tweaked the style a bit, and removed the title that seemed heavy and out of style with the rest of the UI. I noticed a strange issue - if you fire up the website, then load the CS example and then switch back to say arginine, the space occupied by the toggler is not claimed back by the map, even though the div is removed.

Also, once we're done with the polish and the stress-testing, I think it'd be worth adding a sentence or two to the docs somewhere mentioning one can add both types of data to a json, and switch between the two.

@ceriottm Thanks for checking! I pushed the fix for this issue you described with having the correct height back in map.

[ceriottm]

I've one small and one big problem:

• small problem: the fact that this is called "mode" switch is an issue because in python we use "mode" to refer to the type of viewer layout (map/structure/default). this is true in the source and in the settings (that btw still uses "atom" rather than "environment". I suggest we rename mode->target throughout
• bigger problem: I can't seem to be able to create (or interact with) two-mode chemiscopes from jupyter notebooks. it doesn't show the toggle (which is maybe OK, we don't want to clutter too much the jupyter viewers), it doesn't respond to changing the settings "mode" field (which is confusing), and when I export what I believe should be a two-mode chemiscope json, I can't open it.

[sofiia-chorna]

• small problem: the fact that this is called "mode" switch is an issue because in python we use "mode" to refer to the type of viewer layout (map/structure/default). this is true in the source and in the settings (that btw still uses "atom" rather than "environment". I suggest we rename mode->target throughout

@ceriottm Thanks for taking a look!

• small issue:
  I agree that renaming mode to target would help avoid confusion.
  Regarding atom or environment, I am wondering about the consistency between this new target property (which refers to the display mode) and the target property used within properties, which can be either atom or structure.
  If we adopt settings.target with values like environment or structure, should we align the values in settings.properties as well? Specifically, should settings.properties use environment instead of atom to maintain consistency?

• big issue: Thanks for checking it, let me take a look on the fix

[ceriottm]

• small issue:
  I agree that renaming mode to target would help avoid confusion.
  Regarding atom or environment, I am wondering about the consistency between this new target property (which refers to the display mode) and the target property used within properties, which can be either atom or structure.
  If we adopt settings.target with values like environment or structure, should we align the values in settings.properties as well? Specifically, should settings.properties use environment instead of atom to maintain consistency?

Ouch. I didn't realize we were using "atom". This is annoying because I'd say we don't want to lose backward compatibility over this. @Luthaf how would you see using and documenting target: "environment" but accepting also target: "atom"?

[Luthaf]

I'm not sure about target: environment. For most people, the atom/structure distinction is a lot clearer. The "environment" name was initially introduced for displaying atom-centered environments in the structure viewer. Unless there is a good reason to change it, I would keep the "atom" name for target everywhere.

I agree on renaming "mode" to "target", since this matches the name we already use for the properties.

[ceriottm]

ok but then we should be consistent with environments -> atom everywhere. TBD

…

On Tue, 30 Jul 2024 at 17:36, Guillaume Fraux ***@***.***> wrote: I'm not sure about target: environment. For most people, the atom/ structure distinction is a lot clearer. The "environment" name was initially introduced for displaying *atom*-centered environments in the structure viewer. Unless there is a good reason to change it, I would keep the "atom" name for target everywhere. I agree on renaming "mode" to "target", since this matches the name we already use for the properties. — Reply to this email directly, view it on GitHub <#351 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIREZ3ENK3LS3WSHVN4KBLZO6XHVAVCNFSM6AAAAABKRGMHRKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJYGY2DMMBXHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[sofiia-chorna]

@ceriottm @Luthaf
I pushed a small fix to make sure the provided settings correspond to the settings' target.
Besides, here is how I re-install the chemiscope to have the version which is in development in the jupyter notebook:

pip wheel . --no-deps --no-build-isolation --check-build-dependencies --wheel-dir {envtmpdir}/dist
pip install {envtmpdir}/dist/chemiscope-0.7.1-py3-none-any.whl --force-reinstall
jupyter notebook (or restart kernel)

(Let me know if there is a better way, please)

After this re-install, I tested with the example 2-structure_map, seems OK (the display is changing with settings.target property in jupyter).

Please let me know if we need to see the buttons in the jupyter widget.

[ceriottm]

After this re-install, I tested with the example 2-structure_map, seems OK (the display is changing with settings.target property in jupyter).

Please let me know if we need to see the buttons in the jupyter widget.

Do you mean it should change if one changes the settings traitlet, or the it'll display the right thing if one .shows chemiscope with the right target setting? The latter works, the traitlet seems not to

[sofiia-chorna]

Do you mean it should change if one changes the settings traitlet, or the it'll display the right thing if one .shows chemiscope with the right target setting? The latter works, the traitlet seems not to

@ceriottm
Yes I tested by setting the target directly in the settings of .show. I might not fully understand how to change the target using the traitlet then.... Could you please provide an example of how you attempted to change it?

[ceriottm]

chemiscope.show returns an object (which you can show with display(cs))
If you set cs.settings to a valid setting dictionary, it should (and does) automatically update stuff like map axes, coloring, style...
If you do cs.settings={"target":"atom"} it does nothing

[sofiia-chorna]

chemiscope.show returns an object (which you can show with display(cs)) If you set cs.settings to a valid setting dictionary, it should (and does) automatically update stuff like map axes, coloring, style... If you do cs.settings={"target":"atom"} it does nothing

Thanks for explaining!
It does not work for the settings.target but it seems for other settings properties neither. I tried the following:

cs = chemiscope.show(
    frames, properties, environments, settings={
   "map":{
      "x":{
         "property":"atom PCA[1]",
         "scale":"linear"
      },
   },
})

cs
==> plot with x.property = PCA[1] and scale = linear

cs.settings = {
   "map":{
      "x":{
         "property":"atom PCA[2]",
         "scale":"log"
      },
   },
}

cs
==> plot was not changed

Please let me know if I do it correctly and if it does work on your side

[ceriottm]

So, most likely the string was invalid, or the range giving nans (a PCA with a log scale is likely to give problems).
I think we should trigger errors in these cases, but this is probably for another PR. Let's finish this without the traitlet integration and maybe look at better traitlets support in a subsequent PR.

[ceriottm]

We already had a lot of back-and-forth, and I did my own round of polish. This is good to go IMO

[ceriottm]

Just to say I'll merge given that it looks like this works well, and that further tweaks of the logging and/or the traitlet interface will better wait for separate PRs. #1 is fixed!