Update naming of methods for Coulomb logarithms #962
Conversation
There was a problem hiding this comment.
Thank you for working on this! Right now I'm mostly wondering about the balance between abbreviation and readability. The more abbreviated the method names, the harder they are to read and understand without going into the documentation. It'll be necessary to update the tests for Coulomb logarithm too. Thank you again!
|
From helpful discussion with @namurphy, @svincena, and @rocco8773, I think the following should be done.
|
|
Thanks for cleaning this up! |
There was a problem hiding this comment.
Once again, thank you for working on this! Coulomb logarithms turned out to be a lot more complicated than I had originally thought when we first worked on this function.
- My preference at the moment would be to have the abbreviations be
"LS"for classical and"GMS-1"through"GMS-6"for the rest because that's a lot easier to remember. I'm also thinking that if the function will still accept"GMS-1"through"GMS-6", it'd be better to keep that in the documentation. - I think I'd also find it more readable to have underscores like you originally did, like
"ls_full_interp". My suggestions at the moment are:
2. `"ls_min_interp"` or `"GMS-1"`
3. `"ls_full_interp"` or `"GMS-2"`
4. `"ls_clamp_mininterp"` or `"GMS-3"`
5. `"hls_min_interp"` or `"GMS-4"`
6. `"hls_max_interp"` or `"GMS-5"`
7. `"hls_full_interp"` or `"GMS-6"`
Thank you again!
Co-authored-by: Nick Murphy <[email protected]>
|
Hi, Nick. Thank you for your helpful suggestions! I realize now that this PR must have been difficult to review since I made out-of-scope changes. (Sometimes I would get distracted and fix random things.) But yes, I think that this PR is just about done. One question about the unit tests: Should parametrization, rather than separate unit tests, be used for the abbreviations of the methods? @namurphy |
It would be good to have the tests be parametrized, but I'd hold off on refactoring tests until #928 is done since that will add some test helper functionality. I'll go ahead and merge this. Thank you once again! |
namurphy
added
the
plasmapy.formulary
docstrings.
-| Part of Issue #931.
-| Renames the methods for Coulomb logarithms to "LS", "LS_min_I", "LS_full_I", "LS_clamp", "hyp_LS", "hyp_LS_max_I", and "hyp_LS_full_I". Underscores could be removed, "hyp" could be shortened to "H", and "I" could be changed to "i", "interp", or something similar.
-| Other minor updates.