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

DOC: add section on how to write plugin #463

Merged
merged 29 commits into from
Nov 8, 2023
Merged

DOC: add section on how to write plugin #463

merged 29 commits into from
Nov 8, 2023

Conversation

superstar54
Copy link
Member

@superstar54 superstar54 commented Sep 8, 2023

This PR adds the doc for the plugin.

  • Add a development section:
    • architecture page
    • plugin page

@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7224f00) 79.07% compared to head (546e0d0) 79.07%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #463   +/-   ##
=======================================
  Coverage   79.07%   79.07%           
=======================================
  Files          47       47           
  Lines        3211     3211           
=======================================
  Hits         2539     2539           
  Misses        672      672           
Flag Coverage Δ
python-3.10 79.07% <ø> (ø)
python-3.8 79.11% <ø> (ø)
python-3.9 79.11% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@superstar54 superstar54 marked this pull request as draft September 20, 2023 13:46
@superstar54 superstar54 marked this pull request as ready for review September 22, 2023 14:00
Comment on lines 95 to 98
workchain_and_builder = {
"workchain": HelloWorldWorkChain,
"get_builder": get_builder,
}
Copy link
Member

Choose a reason for hiding this comment

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

This is quite an essential specification of the workchain definition. If I understand correctly, the key name and the value type can not be changed. So I would recommend to add a validator to the function who use this value and raise proper exception if the user passes an invalid dictionary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I opened issue #503 for this.

Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

Thanks @superstar54. Here is first go, I add some comments. I think there are more need to change can you first try to add a line break to every sentence so it is easy to review?

docs/source/_static/images/plugin_example.png Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Can you make the description of the plugin the line "A aiidalab-qe plugin to print hello world" indent properly. I know it is the description, but seems like an isolated line. Maybe put a border on the widget?

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed this description line from the plugin.

Wizards UI
==========

Quantum ESPRESSO app (QeApp) uses the Wizards UI, which divides one calculation into four steps. Each step may contain several sections (panels), as shown below.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Quantum ESPRESSO app (QeApp) uses the Wizards UI, which divides one calculation into four steps. Each step may contain several sections (panels), as shown below.
Quantum ESPRESSO app uses the `WizardAppWidget` to divide one calculation into four steps.
Each step can contain several sections (panels), as shown below.

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer to use the Wizards UI instead of the class name WizardAppWidget. Here we give the overview of the architecture instead of detailed code.

docs/source/development/architecture.rst Outdated Show resolved Hide resolved
docs/source/development/architecture.rst Outdated Show resolved Hide resolved
docs/source/development/plugin.rst Outdated Show resolved Hide resolved
docs/source/development/plugin.rst Outdated Show resolved Hide resolved
docs/source/development/plugin.rst Outdated Show resolved Hide resolved
Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

Thanks @superstar54, the reading experience is smooth. I have quite some nitpick changes please have a look. I'd ask @mikibonacci to have a look as well because he is the real user of this new feature.

Two points require discussion:

  • Using the name "Hello world" or "Structure Render" for the demo plugin?
  • Using a gif to show the tabs displayed or two static figures?

I'd just wait for you two to vote and then Xing can start to change, no need to involve more people from the team I guess, better if @AndresOrtegaGuerrero also want to have a look and vote? 😄

Copy link
Member

Choose a reason for hiding this comment

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

This example is not very clear, in the text it says "when the user selects to run the properties." but here the properties are not shown. Could you include the properties list and maybe using two figures to show the differents before and after selecting a new property?
Only if you agree with my suggestion, I think PS and replot the figures are tedious and may not make things better, I'll pining @mikibonacci for comments to see which way is better.

Copy link
Member

Choose a reason for hiding this comment

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

I will vote for Hello world (if we do not use MultiplyAdd instead) and for the gif, as it will be more dynamic and less tedious.

Choose a reason for hiding this comment

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

Sorry for being late, I am pro Hello world since this is more a feature for developers, though i would be more incline to specify something like "Plugin: Custom Property" or "Plugin: Your New Feature" (or property) i think is more related to the idea that each plugin is linked to a new property

docs/source/development/architecture.rst Outdated Show resolved Hide resolved
docs/source/development/architecture.rst Outdated Show resolved Hide resolved
docs/source/development/architecture.rst Outdated Show resolved Hide resolved
docs/source/development/architecture.rst Outdated Show resolved Hide resolved
from aiidalab_qe.common.panel import ResultPanel

class Result(ResultPanel):
title = "Hello world"
Copy link
Member

Choose a reason for hiding this comment

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

In my opinion, "hello world" example is used very specifically for programming language, I guess even aiida plugin didn't use a "hello world" example. Here more specific name could be better. Please consider to change it.

Copy link
Member

Choose a reason for hiding this comment

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

should we use the "MultiplyAddWorkChain" from AiiDA tutorial? https://aiida.readthedocs.io/projects/aiida-core/en/latest/intro/tutorial.html . This is just a detail, but maybe we can make the very same example.

docs/source/development/plugin.rst Outdated Show resolved Hide resolved
docs/source/development/plugin.rst Outdated Show resolved Hide resolved
docs/source/development/plugin.rst Outdated Show resolved Hide resolved
docs/source/development/plugin.rst Outdated Show resolved Hide resolved
@superstar54
Copy link
Member Author

Hi @unkcpz @mikibonacci @AndresOrtegaGuerrero , thanks all for reviewing the PR and suggestions.

I replaced the hello_world plugin with a new example plugin: Equation of State (EOS). I think it is a good example because:

  • it's simple and often used as an example for the first-principles calculation, e.g., in ASE, so many people know.
  • it's also used as a real-world example in AiiDA.
  • it uses almost all the parameters from QeApp core, including the advanced settings; thus, it is a good example to tell developers how to use parameters from the GUI.

I also add examples of the latest features:

  • structure importer and editor
  • result panel for multiple plugins
  • add new code

Please give it another review.

Copy link
Member

Choose a reason for hiding this comment

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

Super cool!

Copy link
Member

@AndresOrtegaGuerrero AndresOrtegaGuerrero left a comment

Choose a reason for hiding this comment

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

Nice work, LGTM!

Copy link
Member

@mikibonacci mikibonacci left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the work, Xing!

Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

Thanks a lot! Some tiny requests, ready to ship then. @superstar54

docs/source/development/architecture.rst Outdated Show resolved Hide resolved
docs/source/development/plugin.rst Outdated Show resolved Hide resolved
docs/source/development/plugin.rst Outdated Show resolved Hide resolved
docs/source/development/plugin.rst Outdated Show resolved Hide resolved
docs/source/development/architecture.rst Show resolved Hide resolved
Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

Looks all good to me. thanks and nice work! @superstar54

@superstar54 superstar54 merged commit 251e423 into main Nov 8, 2023
11 checks passed
@superstar54 superstar54 deleted the doc_plugin branch November 8, 2023 08:53
@unkcpz unkcpz changed the title add plugin page DOC: add section on how to write plugin Nov 9, 2023
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.

None yet

4 participants