Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Baeck-An couplings for SH #186
Baeck-An couplings for SH #186
Changes from all commits
2374cb3
f0fb571
6d3ae69
d34d1d9
f0a3947
d2ade90
918f683
1f4ac58
172acc3
98e235c
9e4d031
98fac22
48e4fa4
de550d1
55dec86
e608c59
3f53c9e
1ecb7ae
3882145
3f7e0dd
1870900
4aa27c8
4e3e36d
f1cc175
adad854
1cf13d1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for introducing the new keywords!
I think we should keep the old variables here for backwards compatibility (so that we don't break existing input files).
Can you also update
sample_inputs/input.in.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
About keeping
inac
andadjmom
, if I keep them in the namelist, someone can set them but they will still be reassigned bycouplings
andmom_adjust
, which have their default values. I'm afraid this would cause more problems. I've never seen input where these two were used so I wouldn't be worried about backwards compatibility of things done in Prague. When you changed the name for decoh_alpha, it was not backwards compatible and still people quickly adapted (again at least here in Prague). So I would stick just to the new keywords, yet I leave the executive decision up to you. :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you could do is to set the
couplings
keyword to some default value, and if the user doesn't change it in the input, don't overwrite the old keywords. I think I already do that foripimd
andmdtype
. But I don't feel too strongly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just coming back to this, I agree it's okay to not support the old keywords and not complicate the code. We need to also document all of this in the documentation, but that's a separate conversation. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note for myself: I don't know why I put the
sh
namelist at the module level, since it is not public, it would be better to put it intosh_init
function, to better separate variables that are only needed there (such as the newcouplings
keyword.Check warning on line 257 in src/surfacehop.F90
Codecov / codecov/patch
src/surfacehop.F90#L257
Check warning on line 271 in src/surfacehop.F90
Codecov / codecov/patch
src/surfacehop.F90#L271
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me if we need to interpolate if inac==2? But better keep the existing behaviour for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is necessary to interpolate but the original code was interpolating also for
inac==2
so I kept it because I didn't want to mess up something.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not stoked about yet another interpolation routine. At some point it would be good to refactor them into a more general routine that could then be called on various arrays. But not in this PR anyway.