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

Remove hard coded codes from workflow #474

Merged
merged 1 commit into from
Sep 21, 2023
Merged

Remove hard coded codes from workflow #474

merged 1 commit into from
Sep 21, 2023

Conversation

superstar54
Copy link
Member

@superstar54 superstar54 commented Sep 19, 2023

The codes (pw_code, dos_code, projwfc_code) are hard coded in the builder, which is not good for the plugin design. This PR uses get_selected_codes.
The get_resources method is also used to make the code more modularized.

@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Patch coverage: 83.33% and no project coverage change.

Comparison is base (edc7b56) 70.45% compared to head (749309b) 70.46%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #474   +/-   ##
=======================================
  Coverage   70.45%   70.46%           
=======================================
  Files          42       42           
  Lines        2955     2966   +11     
=======================================
+ Hits         2082     2090    +8     
- Misses        873      876    +3     
Flag Coverage Δ
python-3.10 70.46% <83.33%> (+<0.01%) ⬆️
python-3.8 70.50% <83.33%> (-0.01%) ⬇️
python-3.9 70.50% <83.33%> (-0.01%) ⬇️

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

Files Changed Coverage Δ
src/aiidalab_qe/app/submission/__init__.py 72.24% <79.16%> (-0.09%) ⬇️
src/aiidalab_qe/plugins/bands/workchain.py 100.00% <100.00%> (ø)
src/aiidalab_qe/plugins/pdos/workchain.py 95.45% <100.00%> (ø)
src/aiidalab_qe/workflows/__init__.py 57.51% <100.00%> (+0.27%) ⬆️

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

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! Just minor requests. @superstar54

# add codes info into input_parameters
self.builder_parameters["codes"] = self.get_selected_codes()
self.builder_parameters["resources"] = self.get_resources()
#
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
#

src/aiidalab_qe/app/submission/__init__.py Show resolved Hide resolved
Comment on lines 364 to 366
"pw_code": self.pw_code.value,
"dos_code": self.dos_code.value,
"projwfc_code": self.projwfc_code.value,
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
"pw_code": self.pw_code.value,
"dos_code": self.dos_code.value,
"projwfc_code": self.projwfc_code.value,
"pw": self.pw_code.value,
"dos": self.dos_code.value,
"projwfc": self.projwfc_code.value,

@@ -134,7 +132,7 @@ def get_builder_from_protocol(
relax_overrides = {"base": parameters["advanced"]}
protocol = parameters["workchain"]["protocol"]
relax_builder = PwRelaxWorkChain.get_builder_from_protocol(
code=pw_code,
code=codes.get("pw_code"),
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
code=codes.get("pw_code"),
code=codes.get("pw"),

@superstar54
Copy link
Member Author

@unkcpz I changed as you suggested. Since we replaced the pw_code with pw, the qeapp.yaml and the workchains are also updated.

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! Looks all good.

I just merge #449, and what to use it as a test. So I'll rebase this PR and see if from PR everything is built correctly. Hope you don't mind.

]:
try:
code_widget = getattr(self, code)
code_widget = getattr(self, f"{code}_code")
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I didn't notice this is directly used in getattr. Then the original code is good enough.

@unkcpz
Copy link
Member

unkcpz commented Sep 21, 2023

Hi @superstar54, after rebasing the tests failed. But I compared the two branches and didn't find the difference. Do you have any clue why it is happening, can you check the changes are all correct?

EDIT: should be fine now, I select incorrect branch when rebase.

replace `pw_code` by `pw`

update qeapp.yaml

f-y
@unkcpz unkcpz changed the title remove hard coded codes Remove hard coded codes from workflow Sep 21, 2023
@unkcpz unkcpz merged commit fba40a5 into main Sep 21, 2023
17 checks passed
@unkcpz unkcpz deleted the refactor_code branch September 21, 2023 22:05
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