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

102 add md calcjob #104

Merged
merged 65 commits into from
Apr 24, 2024
Merged

102 add md calcjob #104

merged 65 commits into from
Apr 24, 2024

Conversation

federicazanca
Copy link
Collaborator

No description provided.

@federicazanca federicazanca self-assigned this Mar 27, 2024
@federicazanca federicazanca added enhancement New feature or request aiida-mlip to help filter the issues labels Mar 27, 2024
@federicazanca federicazanca linked an issue Mar 27, 2024 that may be closed by this pull request
Copy link
Member

@ElliottKasoar ElliottKasoar left a comment

Choose a reason for hiding this comment

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

Out of interest, are you able to run --help or something to list the options, and if so, if that affected by inheritance between the calculations, or is that side of things handled separately?

README.md Outdated Show resolved Hide resolved
aiida_mlip/calculations/base.py Show resolved Hide resolved
aiida_mlip/calculations/base.py Outdated Show resolved Hide resolved
@ElliottKasoar
Copy link
Member

ElliottKasoar commented Mar 28, 2024

Out of interest, are you able to run --help or something to list the options, and if so, if that affected by inheritance between the calculations, or is that side of things handled separately?

To clarify this a bit better, a few examples of shared arguments for typer are discussed here.

With janus, for example, if I defined the shared architecture option via the callback method proposed in the issue above, the option would be listed under janus --help, but not under janus singlepoint --help, which is not ideal.

Clearly the CLI is built quite differently with click, classes, etc. here, but I can imagine an issue that looks similar could occur - the help may not include inherited options.

It's probably fine, but it's something to check.

@federicazanca
Copy link
Collaborator Author

federicazanca commented Apr 4, 2024

Some things to do still

  • make some sort of results_dict
  • README file/docs
  • Maybe I need some tests for the base scripts I created
  • check the --help thing Elliot said

But feel free to start commenting on the code as these thing won't modify (I think) what I've done so far

pyproject.toml Outdated Show resolved Hide resolved
aiida_mlip/calculations/base.py Outdated Show resolved Hide resolved
aiida_mlip/calculations/md.py Outdated Show resolved Hide resolved
aiida_mlip/calculations/md.py Outdated Show resolved Hide resolved
aiida_mlip/calculations/singlepoint.py Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@ElliottKasoar
Copy link
Member

Just a note that there will be minor changes to the janus CLI once stfc/janus-core#100 is merged to allow reading a config.

The currently linked janus-core will be fine, but if we updated that before merging this, we'd need to take it into account.

In most cases I changed the Python variables to match the CLI option names, so the CLI is largely unchanged, but there is at least one difference: --property -> --properties (since property is a Python keyword)

README.md Outdated Show resolved Hide resolved
@federicazanca federicazanca marked this pull request as ready for review April 8, 2024 14:55
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
tests/helpers/test_load.py Outdated Show resolved Hide resolved
tests/helpers/test_converters.py Outdated Show resolved Hide resolved
tests/calculations/test_singlepoint.py Outdated Show resolved Hide resolved
tests/calculations/test_md.py Outdated Show resolved Hide resolved
tests/calculations/test_md.py Outdated Show resolved Hide resolved
aiida_mlip/parsers/md_parser.py Outdated Show resolved Hide resolved
ElliottKasoar
ElliottKasoar previously approved these changes Apr 22, 2024
Copy link
Member

@ElliottKasoar ElliottKasoar left a comment

Choose a reason for hiding this comment

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

Optional suggestion, but otherwise I think this looks good.

We'll revisit a lot of the option names, config etc. anyway at some point.

docs/source/user_guide/calculations.rst Outdated Show resolved Hide resolved
oerc0122
oerc0122 previously approved these changes Apr 22, 2024
ElliottKasoar
ElliottKasoar previously approved these changes Apr 23, 2024
oerc0122
oerc0122 previously approved these changes Apr 23, 2024
@alinelena alinelena merged commit d24ffd4 into stfc:main Apr 24, 2024
8 checks passed
@federicazanca federicazanca deleted the 102-add-md-calcjob branch April 26, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aiida-mlip to help filter the issues enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add option config file add test for helper add MD calcjob
4 participants