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

Hide the Cell tab when the structure is not periodic #351

Open
unkcpz opened this issue Sep 6, 2022 · 13 comments · May be fixed by #522
Open

Hide the Cell tab when the structure is not periodic #351

unkcpz opened this issue Sep 6, 2022 · 13 comments · May be fixed by #522
Labels
enhancement New feature or request
Milestone

Comments

@unkcpz
Copy link
Member

unkcpz commented Sep 6, 2022

The StructureDataviewer will show the cell tab if the cell tab is initialized when creating the viewer.
It makes no sense to show the cell information for the non-periodic structure.
So when the structure detected is not 0-D structure the cell tab should be completely hidden.

But for the 1-D and 2-D structures, there are options here, either we can hide the cell tab completely or we can show partial information with the cell.

@unkcpz unkcpz added the enhancement New feature or request label Sep 6, 2022
@danielhollas danielhollas added this to the v.2.1.0 milestone Feb 24, 2023
@AndresOrtegaGuerrero
Copy link
Member

#488 address this ? , if you test now the viewer do not hide the cell parameters for 1D and 2D periodicity

@unkcpz
Copy link
Member Author

unkcpz commented Sep 26, 2023

Yep! thanks!

@unkcpz unkcpz closed this as completed Sep 26, 2023
@danielhollas
Copy link
Contributor

@AndresOrtegaGuerrero are you sure? I think this issue is about something else than what was addressed in #488. Here the idea was that the Cell tab should be completely hidden when the structure is not periodic. I don't think that's the case now?

@AndresOrtegaGuerrero
Copy link
Member

@danielhollas if the behaviour desired is like that , then no, it doesnt hide the cell tab when having a 0D material, but regarding 1D and 2D materials it doesnt disappear the cell parameters

@danielhollas danielhollas reopened this Sep 26, 2023
@unkcpz
Copy link
Member Author

unkcpz commented Sep 26, 2023

Emm... @danielhollas is right, I didn't read carefully what I wrote before, sorry. If the structure is a molecule, the tab will still show. But the behavior is more confusing now. I upload a water molecule and there are values for the cell. Not sure where it comes from.

Peek 2023-09-26 17-56

The structure is from https://github.com/susilehtola/erkale/blob/master/tests/xyz/H2O.xyz

@danielhollas
Copy link
Contributor

I think that's because we add a dummy cell to gas phase molecule. In the old AiiDA, there needed to be default cell, otherwise StructureData would not work. But that has been fixed so we should get rid of this login in AWB.

https://github.com/aiidalab/aiidalab-widgets-base/blob/master/aiidalab_widgets_base/structures.py#L379

@danielhollas
Copy link
Contributor

The fix in aiida-core was done here aiidateam/aiida-core#5341
and released in version 2.0 https://github.com/aiidateam/aiida-core/blob/main/CHANGELOG.md#v200---2022-04-27

@danielhollas
Copy link
Contributor

@unkcpz can you try if it works correctly if you generate a molecule from SMILES?

@unkcpz
Copy link
Member Author

unkcpz commented Sep 26, 2023

I'll try it later, I only have QE app at hand which doesn't have SMILES tab open 😂

@AndresOrtegaGuerrero
Copy link
Member

AndresOrtegaGuerrero commented Sep 27, 2023

I think a future PR should be to add a widget to a cell editor, in such a way that first it can change and set the periodicity of the system (we have one in QE_app). Given this widget it should control if the cell appears or disappear (It could also just set the layout of the cell tab to say the system has periodicity none, but display the symmetry information, relevant for the molecule) based on the periodicity, and then the user can set the cell parameters if desired.

@unkcpz
Copy link
Member Author

unkcpz commented Sep 28, 2023

@unkcpz can you try if it works correctly if you generate a molecule from SMILES?

image

But that has been fixed so we should get rid of this login in AWB.

@danielhollas I think it is al fine, which means we can safely remove this function I assume?

But for the SMILE widget itself, I first encounter rdkit is not installed and then after I install the rdkit from conda, I encountered sklearn not installed.
I think we need to make those extra dependencies for smile.

@danielhollas
Copy link
Contributor

Yes, I think we should remove that function. btw: SMILES-generated molecules also should not have any cell, not sure where that comes from.

But for the SMILE widget itself, I first encounter rdkit is not installed and then after I install the rdkit from conda, I encountered sklearn not installed. I think we need to make those extra dependencies for smile.

That's already done in setup.cfg as extra dependencies (RDkit and sklearn are big so we do not want them to be installed by default. You can install them as pip install .[smiles]

@unkcpz unkcpz linked a pull request Oct 4, 2023 that will close this issue
1 task
@unkcpz
Copy link
Member Author

unkcpz commented Oct 4, 2023

I thought this was not hard to solve, but after some attempts (#521 #522 ), this requires more tidy up on the StrucutreDataViewer and therefore goes with other on going PRs related to structure viewer.
I move this issue to milestone v2.2.0.

@unkcpz unkcpz modified the milestones: v2.1.0, v2.2.0 Oct 4, 2023
@danielhollas danielhollas modified the milestones: v2.2.0, v0.2.3 Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants