AstarVienna / ScopeSim_Templates

GNU General Public License v3.0
2 stars 4 forks source link

Minor improvements to stellar module #57

Closed teutoburg closed 1 year ago

teutoburg commented 1 year ago

Mostly related to cluster functionality, see commit msgs for details.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 91.83% and project coverage change: +0.13% :tada:

Comparison is base (bcf5590) 78.80% compared to head (6cb4a2a) 78.94%. Report is 2 commits behind head on dev_master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev_master #57 +/- ## ============================================== + Coverage 78.80% 78.94% +0.13% ============================================== Files 43 43 Lines 1958 1952 -6 ============================================== - Hits 1543 1541 -2 + Misses 415 411 -4 ``` | [Files Changed](https://app.codecov.io/gh/AstarVienna/ScopeSim_Templates/pull/57?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AstarVienna) | Coverage Δ | | |---|---|---| | [scopesim\_templates/stellar/imf.py](https://app.codecov.io/gh/AstarVienna/ScopeSim_Templates/pull/57?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AstarVienna#diff-c2NvcGVzaW1fdGVtcGxhdGVzL3N0ZWxsYXIvaW1mLnB5) | `48.15% <20.00%> (ø)` | | | [scopesim\_templates/stellar/cluster\_utils.py](https://app.codecov.io/gh/AstarVienna/ScopeSim_Templates/pull/57?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AstarVienna#diff-c2NvcGVzaW1fdGVtcGxhdGVzL3N0ZWxsYXIvY2x1c3Rlcl91dGlscy5weQ==) | `100.00% <100.00%> (+9.52%)` | :arrow_up: | | [scopesim\_templates/stellar/clusters.py](https://app.codecov.io/gh/AstarVienna/ScopeSim_Templates/pull/57?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AstarVienna#diff-c2NvcGVzaW1fdGVtcGxhdGVzL3N0ZWxsYXIvY2x1c3RlcnMucHk=) | `100.00% <100.00%> (ø)` | | | [scopesim\_templates/stellar/stars.py](https://app.codecov.io/gh/AstarVienna/ScopeSim_Templates/pull/57?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AstarVienna#diff-c2NvcGVzaW1fdGVtcGxhdGVzL3N0ZWxsYXIvc3RhcnMucHk=) | `97.36% <100.00%> (-0.04%)` | :arrow_down: | | [scopesim\_templates/stellar/stars\_utils.py](https://app.codecov.io/gh/AstarVienna/ScopeSim_Templates/pull/57?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AstarVienna#diff-c2NvcGVzaW1fdGVtcGxhdGVzL3N0ZWxsYXIvc3RhcnNfdXRpbHMucHk=) | `100.00% <100.00%> (ø)` | | | [...templates/tests/test\_stellar/test\_cluster\_utils.py](https://app.codecov.io/gh/AstarVienna/ScopeSim_Templates/pull/57?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AstarVienna#diff-c2NvcGVzaW1fdGVtcGxhdGVzL3Rlc3RzL3Rlc3Rfc3RlbGxhci90ZXN0X2NsdXN0ZXJfdXRpbHMucHk=) | `95.23% <100.00%> (-0.69%)` | :arrow_down: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

hugobuddel commented 1 year ago

Great! LGTM.

A thought about renaming of mass2Mv() to mass2absmag(): it can also be useful to use absmag_from_mass(), as the _from_ naming convention makes it easier to visually parse that code is correct than the _to_ convention. Compare:

my_mag = absmag_from_mass(my_mass)
...
my_mag = mass_to_absmag(my_mass)

In the first, the part of the function name that refers to the output of the function is close to where the output actually goes, and the part of the function name that refers to the input is close to the input of the function.

I've been using the _from_ convention since reading https://www.joelonsoftware.com/2005/05/11/making-wrong-code-look-wrong/

Nevertheless, the new name is an improvement over the original name (and perhaps more conventional then my suggestion), and this is just bikeshedding.