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

Initial definitions for simulation workflow #121

Open
wants to merge 25 commits into
base: develop
Choose a base branch
from

Conversation

JosePizarro3
Copy link
Collaborator

@JosePizarro3 JosePizarro3 commented Sep 19, 2024

@ladinesa @JFRudzinski this is my attempt to move some classes and simplify stuff from the simulation workflow package. Sorry for the changes, I also moved testing files to subfolders (so please, disregard the changes for the subfolders tests/properties).

Some list of changes:

  • I moved SimulationWorkflow, SinglePoint, BeyondDFT and DFTPlusTB
  • I simplified quite a lot the logic in the original classes. I think this is a reflection of the inheritance hell one can enter, and I got really crazy trying to test stuff (you can see in Defining SimulationWorkflow, SinglePoint, BeyondDFT and DFTPlusTB workflows #120 my failed attempt on testing the original logics...). In any case, I think we can rework things along the development process when needed.
  • I did not move SerialWorkflow and ParallelWorkflow. Also related with the inheritance tight coupling. But we can recover them if needed later on.
  • I added an utils function to extract the main subsections of the archive.data.

When you want, you can review it. In the meantime, I will test the Wannier90 parser with this slightly new DFTPlusTB workflow section.

@coveralls
Copy link

coveralls commented Sep 19, 2024

Pull Request Test Coverage Report for Build 11269921175

Details

  • 131 of 142 (92.25%) changed or added relevant lines in 5 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.7%) to 81.085%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/nomad_simulations/schema_packages/workflow/base_workflows.py 51 53 96.23%
src/nomad_simulations/schema_packages/workflow/single_point.py 22 24 91.67%
src/nomad_simulations/schema_packages/workflow/dft_plus_tb.py 45 48 93.75%
src/nomad_simulations/schema_packages/utils/utils.py 10 14 71.43%
Files with Coverage Reduction New Missed Lines %
src/nomad_simulations/schema_packages/utils/utils.py 1 78.57%
Totals Coverage Status
Change from base Build 11101838327: 0.7%
Covered Lines: 2182
Relevant Lines: 2691

💛 - Coveralls

@JosePizarro3 JosePizarro3 changed the title 100 different structure classes Initial definitions for simulation workflow Sep 20, 2024
@JosePizarro3 JosePizarro3 force-pushed the 100-different-structure-classes branch from 64ae0ef to 6a7668b Compare October 2, 2024 08:11
Copy link
Collaborator

@JFRudzinski JFRudzinski left a comment

Choose a reason for hiding this comment

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

Maybe I will have more comments when I work on the MD part, but think the adjustments look fine to me at this point

@JosePizarro3
Copy link
Collaborator Author

@ladinesa I might need to merge this soon so the TMM can test stuff. Can you review it? I know the refs are bugged, but that's another issue, not related with the schema.

super().normalize(archive, logger)


class BeyondDFTMethod(ArchiveSection):
Copy link
Collaborator

Choose a reason for hiding this comment

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

No base class for method?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, this is just a place holder for referencing DFT, TB, DMFT, GW... sections in the archives of my workflows.


@check_n_tasks(n_tasks=2)
def link_task_inputs_outputs(
self, tasks: list[TaskReference], logger: 'BoundLogger'
Copy link
Collaborator

@ladinesa ladinesa Oct 8, 2024

Choose a reason for hiding this comment

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

i thought you already figured out how to handle loggers. since you are using decorators, maybe create a decorator for error handling as nathan suggested

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, the loggers issue is after this one. I can change to the class __init__ ; I find it more convenient to use self.logger for all methods

tasks=self.tasks,
tasks_names=['DFT SinglePoint Task', 'TB SinglePoint Task'],
)
if method_refs is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

here, method_refs should only length 2 right and arranged as DFT, TN so maybe loop not needed

return None

# Input of DFT Task is the ModelSystem
dft_task.inputs = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure if i understood this right but it should be the other way around right? i.e you assign self.in(out)puts from dft and tb task inputs and outputs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After our discussion, this is what was confusing me with the need of defining inputs/outputs in a TaskReference.

I think I can improve this in a new schema as specified in https://gitlab.mpcdf.mpg.de/nomad-lab/nomad-FAIR/-/merge_requests/2143 with some minor tweeks

@ladinesa
Copy link
Collaborator

ladinesa commented Oct 8, 2024

looks good just minor clarifications needed

@JosePizarro3
Copy link
Collaborator Author

@ladinesa thanks! I have another branch I will eventually merge here, I took into account your comments.

I have a question tho: right now, the Wannier90 parser splits the entries for TB SinglePoint and DFTPlusTB. They share the same mainfile, do you think that's why the references are broken? Should I do something different when storing stuff in Link and TaskReference?

@ladinesa
Copy link
Collaborator

@ladinesa thanks! I have another branch I will eventually merge here, I took into account your comments.

I have a question tho: right now, the Wannier90 parser splits the entries for TB SinglePoint and DFTPlusTB. They share the same mainfile, do you think that's why the references are broken? Should I do something different when storing stuff in Link and TaskReference?

but this is the idea of child archives and it should work

@JosePizarro3
Copy link
Collaborator Author

@ladinesa thanks! I have another branch I will eventually merge here, I took into account your comments.
I have a question tho: right now, the Wannier90 parser splits the entries for TB SinglePoint and DFTPlusTB. They share the same mainfile, do you think that's why the references are broken? Should I do something different when storing stuff in Link and TaskReference?

but this is the idea of child archives and it should work

Ok, then nvm :)

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.

4 participants