BrianLi009 / PhysicsCheck

Other
5 stars 3 forks source link

Typo when main.sh calls verify.sh #2

Closed curtisbright closed 2 years ago

curtisbright commented 2 years ago

https://github.com/BrianLi009/private_ks/blob/c7c1e0abd5bc3211547e453f18d15ff485742335/main.sh#L134

Hi Brian, the verification script will never successfully be called because there is a typo in its name.

The fact issues like this occur whenever I try running the script makes me wonder how/if the script is tested. An error will occur every time the script is called, so I would expect the issue to be noticed the very first time the script is run -- yet somehow it went undetected while running experiments. Is this because you are not running main.sh but some other script, or you are running main.sh but not checking output for errors? Either way, going forward I think it's best that script testing and running experiments happen simultaneously. Thanks Brian.

BrianLi009 commented 2 years ago

Hi Curtis, thanks for pointing this out. I have made the changes and ran main.sh to make sure it works. I am currently on the trip, so I will run it more thoroughly soon, with more changes in parameters to make sure it is okay.

The reason these issues keep occuring, is that, currently on Computer Canada, the verification script and embedability checkings are being commented out. The reason is that these steps are not needed to obtain the simplification and solving time data. After I implemented verify.sh, this part of the main.sh was never ran on Compute Canada.

Thanks for the suggestions, I am still learning and working to get rid of bad software engineering practices.

curtisbright commented 2 years ago

Thanks Brian. Another thing I noticed is that your commits should only include changes relevant to a single update, but often your commits modify or add unnecessary files. For example, the most recent commit also adds strange "dos2unix" calls in dependency-setup.sh. Those calls shouldn't be necessary, but even if they are they should be added in a separate commit. You can use "git add" to only stage specific files to be committed and "git add -p" if you only want to stage specific changes in specific files.

BrianLi009 commented 2 years ago

Hi Curtis, I'll do that from now on. The reason I wanted to include dos2unix calls is that, there are always hidden windows characters when I push to the repo, since I am on a windows system. As a result, if you directly pull on a Linux machine and run main.sh, the hidden characters will cause an error.

Is there a better way of doing it other than dos2unix call on every single script file? Thanks.

curtisbright commented 2 years ago

Hi Brian, this is something that you should configure in the editor you are using to edit the scripts. By default Windows systems have an \r (carriage return) character before \n (newline) characters. However, many editors will either have an option to use Unix line-endings or detect the type of file you are editing and behave accordingly.