Skip to content

FIX: Read velocities from file in r.terabin#124

Merged
danielhollas merged 2 commits intomasterfrom
fix-vel-terabin
Apr 27, 2022
Merged

FIX: Read velocities from file in r.terabin#124
danielhollas merged 2 commits intomasterfrom
fix-vel-terabin

Conversation

@danielhollas
Copy link
Copy Markdown
Contributor

@danielhollas danielhollas commented Apr 26, 2022

Unfortunately, r.terabin has been broken since Dec 2019 since this commit:

42eec0b#diff-9be526330a9213578d1f49c8c02341eb1a4474cd3aa8de81cda0a1051417f7d0R128

It would ignore the velocity file specified at the top in variable $VELOC_IN.

@suchanj I am not sure how much is this script used and how much are people reading velocities from a file...

This bug would should have been caught earlier, since for simulations without velocity file we were executing ABIN lwith an empty -v like this:

abin -i input -x geom -v

Unfortunately, due to the way we were parsing the command line, this was interpreted as an empty string, and because the default for the velocity file name is an empty string, this was a valid invocation. :-( I am fixing this here and explicitly checking for empty arguments.

I hate BASH 😢

@danielhollas danielhollas self-assigned this Apr 26, 2022
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 26, 2022

Codecov Report

Merging #124 (a2fd601) into master (9db3c5c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #124   +/-   ##
=======================================
  Coverage   91.56%   91.56%           
=======================================
  Files          42       42           
  Lines        5783     5788    +5     
=======================================
+ Hits         5295     5300    +5     
  Misses        488      488           
Flag Coverage Δ
unittests 23.05% <0.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/cmdline.F90 100.00% <100.00%> (ø)

@danielhollas danielhollas requested a review from suchanj April 26, 2022 18:40
Comment thread utils/r.terabin
ABIN_CMD="$ABIN_CMD -M $TC_PORT"
fi
if [[ -z $VELOC_IN ]];then
if [[ -n $VELOC_IN ]];then
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the fix. :-(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ugh, ok not great, but I think a) most of the people were not going to the $PHOTOX for the script b) tera was kinda underused for the last few years. Nevertheless, we should consider making ABIN 1.1 default for the older clusters to catch this error.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, for sure. I am basically done with the refactorings and testing. I need to fix some issues with restart (especially for SH). I wil then run a set of SH calculations for Basile's PCCP test systems and the we should release version 1.2

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great! 💥

@yoricklassmann
Copy link
Copy Markdown

@danielhollas can confirm that your fix works as intended, thanks a lot! Let's just hope this bug doesn't affect too many people.

Comment thread tests/CMDLINE/test.sh
mv ERROR ABIN_ERROR4

$ABINEXE -M >> abin.out 2>&1 || true
mv ERROR ABIN_ERROR5
Copy link
Copy Markdown
Contributor

@suchanj suchanj Apr 27, 2022

Choose a reason for hiding this comment

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

Nice! (testing)

@danielhollas danielhollas merged commit e0bb962 into master Apr 27, 2022
@danielhollas danielhollas deleted the fix-vel-terabin branch April 27, 2022 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants