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

Add from_nodes class method to instantiate a BandsPdosModel from AiiDA nodes #1037

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

edan-bainglass
Copy link
Member

This PR adjusts the bands-pdos MVC component by removing the required bands/pdos AttributeDict nodes from the widget constructor and adding a from_nodes class method to the model that takes care of the AttributeDict extraction. This is one in several future PRs that aims to both make this MVC component more usable outside of the app (for local plotting of the electronic structure), and prepare the component for the future unification of electronic structure results tabs.

@edan-bainglass edan-bainglass force-pushed the bands-pdos-model-from-nodes branch from 1bc45fe to ea6a842 Compare December 26, 2024 08:20
@edan-bainglass
Copy link
Member Author

@AndresOrtegaGuerrero a colleague was complaining quite a bit about not being able to use the app's widgets to quickly plot some electronic structure stuff. This PR is trying to address this, while also starting to address #818. At the moment, I am providing the means to easily create the MVC component from workflow nodes. I will probably commit a change in a bit to allow also from the calculation nodes, which is the colleague's case.

@@ -7,31 +7,9 @@


class BandsPdosWidget(ipw.VBox):
"""
A widget for plotting band structure and projected density of states (PDOS) data.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are removing mos of the Docstring, add it where it should be , so is easy for someone in the future to do changes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, but later, once we understand the design better 👍 This is not done.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same goes for tests. No need to comment on them not passing. I'll adjust w.r.t the final design.

@edan-bainglass edan-bainglass self-assigned this Dec 26, 2024
@@ -79,6 +82,37 @@ def __init__(self, *args, **kwargs):
lambda _: self._has_pdos or self.needs_projections_controls,
)

@classmethod
def from_nodes(
cls,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should include some doc string here, to explain why these decisions, and that the bands conditionals is to guarantee backwards compatibility

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Once the design is settled, docstrings 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants