Closed sofiia-chorna closed 1 month ago
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.
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 thanAtoms
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
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.
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.
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
@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
- small issue: I agree that renaming
mode
totarget
would help avoid confusion. Regarding atom or environment, I am wondering about the consistency between this newtarget
property (which refers to the display mode) and the target property used withinproperties
, which can be eitheratom
orstructure
. If we adoptsettings.target
with values likeenvironment
orstructure
, should we align the values insettings.properties
as well? Specifically, shouldsettings.properties
useenvironment
instead ofatom
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"?
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.
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 https://github.com/lab-cosmo/chemiscope/pull/351#issuecomment-2258646078, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIREZ3ENK3LS3WSHVN4KBLZO6XHVAVCNFSM6AAAAABKRGMHRKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJYGY2DMMBXHA . You are receiving this because you were mentioned.Message ID: @.***>
@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.
After this re-install, I tested with the example
2-structure_map
, seems OK (the display is changing withsettings.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
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?
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
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
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.
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!
Where to play around with modes:
Open the
Chemical Shieldings
exampleOpen
examples/2-structure_map.html
TODO:
After review:
Display errors as errors instread of warning--> throw error, change logging in separate PRmode
to the settingsNote for exporting/importing of the dataset:
z.min
andz.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).