Add path calculation and Fit to NEB plot#11
Conversation
|
@hampusnasstrom @u-gajera I have finished now my suggestion for the basic workflow.
|
Pull Request Test Coverage Report for Build 16145787267Details
💛 - Coveralls |
|
FYI @JFRudzinski |
JFRudzinski
left a comment
There was a problem hiding this comment.
@schumannj This is a really great idea. I imagine this will be very helpful to users.
I would just suggest a few changes to avoid restricting the normalization too much. As it is now, creation of the full neb workflow from a yaml or from a parser in a custom way is no longer possible.
I think this probably makes extensions/generalizations in the future more difficult.
HERE is a PR where I am adding functionalities in the normalization, and there may be some useful things there for reference.
I will consider there whether I want to implement something similar to what you have done here.
Finally, at minimum there should be a detailed description of what this normalization will do in different cases. In a follow up PR you could add very small docs page maybe and link it to the repo, so that we could send people there to understand how to create the yaml correctly
|
@JFRudzinski thanks a lot for your review. These are very valuable comments. I have looked briefly at the PR you tagged. The materials card in the workflow entry is brilliant. I was wondering how we get the structures (or at least one representative structure) into the workflow entry. I will have a closer look at how you did it and address the rest of the comments soon. |
|
@schumannj I just wanted to share an idea that came up today in a meeting I had with Martin Kuban: we were discussing a similar workflow normalizer approach to the Alexandria data workflows. It became clear that to improve future adaptability, the task sections could simply point to the main file. Then the additional archive section path can be set by the normalizer. This way If the archive schema changes in the future (as will be the case soon for the run section) the normalizer can simply be updated and the workflow entry reprocessed. What do you think? @hampusnasstrom thoughts? |
|
@JFRudzinski Does make sense to me, I will try it out. |
ok great! I am also going to try this approach in my EOS workflow class. I will share here when I have something for reference |
Ok, I am just realizing that my suggested approach may not be so straightforward. I think in the end it may require some adjustment in the metainfo(yaml) parser, or whatever is controlling the workflow instantiation. I need to talk to Alvin about this, but I think you should completely disregard for now...sorry if I wasted some of your time! |
|
@schumannj ok, I talked to Alvin today, and it seems like what I suggested is not really a great approach in the end because there are already certain restrictions in the metainfo parser that we should change for specific implementations. Instead, we would sometime in the future plan to create a new "specialized metainfo parser" for workflows which performs some additional magic on a simplified version of the current yaml schema. I hope that makes sense, but we can discuss further at some point. until then I think you can continue with your previous approach. Again sorry for sending you astray! |
…s, improve docstrings, bit of clean up and add system images to overview
|
@JFRudzinski @hampusnasstrom @ladinesa @u-gajera The preferred workflow file looks like this: workflow2:
m_def: nomad_neb_workflows.schema_packages.neb.NEBWorkflow
name: Battery Example NEB workflow
inputs:
- name: Input for battery example NEB
section: '../upload/archive/mainfile/AlCo2S4/neb/00/OUTCAR#/'
- name: Input structure for image 6
section: '../upload/archive/mainfile/AlCo2S4/neb/05/OUTCAR#/'
tasks:
- name: Input structure for image 2
section: '../upload/archive/mainfile/AlCo2S4/neb/01/OUTCAR#/'
- name: Input structure for image 3
section: '../upload/archive/mainfile/AlCo2S4/neb/02/OUTCAR#/'
- name: Input structure for image 4
section: '../upload/archive/mainfile/AlCo2S4/neb/03/OUTCAR#/'
- name: Input structure for image 5
section: '../upload/archive/mainfile/AlCo2S4/neb/04/OUTCAR#/' |
|
Let me know what you think. |
JFRudzinski
left a comment
There was a problem hiding this comment.
Hi @schumannj , sorry for the delay!
I think this looks really great. Tbh I didn't do the MOST thorough code review, but from a glance it looks very good. It has been a while now since I was working on my workflow normalizer, but when I get back to it (possibly next week or maybe in 2), I will be looking closely at your code to adopt some aspects. So, I will give further comments if anything comes up. However, I don't think this should keep you from merging, you can always patch any small thing later, the core functionality seems to be complete.
I would just either 1. directly resolve comments if no longer relevant, 2. adjust and resolve, 3. or open an issue / add a TODO if it is something you don't want/need to address now (one definite issue I would add is a note to include docs that describe the YAML usage). Then from my side it is good to go!
|
Thanks a lot @JFRudzinski! I will try to add some basic documentation and then merge. |
|
@schumannj I suggest that we merge this as is. Further documentation or other adjustments can always be made later. I would then also like to merge my PR in nomad-docs where I added the short description and link to these doc pages. Is this all fine with you? |
Didn't realize you were definitely out this week+, we decided to go ahead with the merge. We can always adjust things when you are back. Thanks for all the great work here!! |


I have added and changed a couple of things: