BattModels / asimtools

Optimized workflow management and script handling for atomistic simulations
MIT License
2 stars 0 forks source link

Emil make package and add console script #5

Closed emilannevelink closed 1 year ago

emilannevelink commented 1 year ago

Major queries:

  1. Is there a reason for moving scripts into the inner asimtools directory?

This was just how I knew to add it to the path when it is installed. Is there another way that doesn't require adding the path in your environment.

  1. Doesn't look like you're using pylint? I fixed any aesthetic formatting issues I found

Whoops, sorry. I hadn't loaded it on arjuna.

Changes I made:

README.md

22: Recommend using pip install -e . This installs in developer mode so you don't have to reinstall each time you make a change. Minor changes to make it work for a first-timer Great

job.py

  • 282: Clean up debugging print statements
  • 287: Renamed output files to stdout.txt and stderr.txt since that's exactly what they are and output might get overloaded
  • Followed Linting suggestions

Thanks

asim_run.py

14: Arguments can either be on one line or in the form:

def my_func( arg1, arg2, ): ...

I don't get what you changed here

so that one line corresponds to one argument 14: Changed calc_inputfile -> calc_input_file etc. and added a bit more for help argument

40: Wasn't working for loading arbitrary scripts given path. fixed it following https://stackoverflow.com/questions/67631/how-can-i-import-a-module-dynamically-given-the-full-path

Tests needed

  1. Loading built-in and arbitrary scripts using asim-run

I'll work on this tomorrow

  1. Parsing arguments? Not sure if this is common to test

It might be good to just have a single parse command line function instead of in each file. Then I think we can make a test for it.

asim_execute.py

  • Deleted
mkphuthi commented 1 year ago

I don't get what you changed here

It was just some formatting to make functions with lots of arguments neater

It might be good to just have a single parse command line function instead of in each file. Then I think we can make a test for it.

Actually, originally I had the parse_arguments in utils.py. It still is there but I figured if only asim_run.py uses it, it should only be in there. We should pick one or the other.

emilannevelink commented 1 year ago

I added tests for asim_run.py for the singlepoint calculation. I don't know how we would want to test a script that isn't in the repository.

I also added a test for parse_command_line. Let me know if the format of these tests make sense. I am pretty inexperienced when it comes to developing tests, so I don't know if this is the right way of doing them.

mkphuthi commented 1 year ago

Summary:

Major Changes:

Minor Changes: