feat: ability to run global-evolution through CLI#621
feat: ability to run global-evolution through CLI#621steven-murray wants to merge 1 commit intorelease-v4.2from
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release-v4.2 #621 +/- ##
===============================================
Coverage ? 88.01%
===============================================
Files ? 32
Lines ? 4755
Branches ? 803
===============================================
Hits ? 4185
Misses ? 410
Partials ? 160 ☔ View full report in Codecov by Sentry. |
|
|
||
| Parameters | ||
| ---------- | ||
| redshift_range |
There was a problem hiding this comment.
I think that redshift_range doesn't have any impact on run_global_evolution, only on run_lightcone (I couldn't also see any difference when I played with this parameter with the new cli). Is there a need to define redshift_range in this function?
|
|
||
|
|
||
| @run.command(name="global") | ||
| def global_signal( |
There was a problem hiding this comment.
It doesn't matter too much, but I think a better name is global_evolution, rather than global_signal, since this function doesn't compute only the global 21cm signal, but also the global history of all the other fields in the simulation.
The important thing is that we can access this function via global in the cli, which is great :)
jordanflitter
left a comment
There was a problem hiding this comment.
Thanks @steven-murray for adding this feature! It is probably better to add it to v4.2 since it is a new (minor) feature, but I guess it's also okay to include it a new patch for v4.1, so I leave the decision to you :)
I played a bit with this new feature with the cli and overall things seem to work smoothly. Besides the comment/question that I had about redshift-range, my other comment is that we should probably spend some words on this new feature in cli_usage.rst. Also, I'm surprised that you didn't include this feature in a new test in test_cli.py (I don't understand how codecov didn't fail 😅)
Fixes #614
@jordanflitter I suggest taking this for a spin with some examples to make sure everything acts as you think it should.
I'm proposing we merge this into a "v4.2" branch because it's technically a new feature and so should bump minor version, but I think maybe we want to include some other features in v4.2 as well, and don't want to stop bug fixes from going into v4.1.
It is a very small feature though, so we could pass it off as a patch version if you think that's OK.