spacetelescope / astrocut

Tools for making image cutouts from sets of TESS full frame images
https://astrocut.readthedocs.io
25 stars 12 forks source link

Documenting multithreading enhancement #86

Closed jaymedina closed 1 year ago

jaymedina commented 1 year ago

This PR updates the Astrocut documentation to reflect the work done in PR https://github.com/spacetelescope/astrocut/pull/84

These are the new changes:

image
codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.61 :tada:

Comparison is base (311488d) 93.78% compared to head (2c9fdd6) 94.40%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #86 +/- ## ========================================== + Coverage 93.78% 94.40% +0.61% ========================================== Files 8 8 Lines 1416 1429 +13 ========================================== + Hits 1328 1349 +21 + Misses 88 80 -8 ``` | [Impacted Files](https://app.codecov.io/gh/spacetelescope/astrocut/pull/86?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope) | Coverage Δ | | |---|---|---| | [astrocut/cube\_cut.py](https://app.codecov.io/gh/spacetelescope/astrocut/pull/86?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope#diff-YXN0cm9jdXQvY3ViZV9jdXQucHk=) | `98.93% <100.00%> (+0.28%)` | :arrow_up: | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/spacetelescope/astrocut/pull/86/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

scfleming commented 1 year ago

"To use multiprocessing, set the threads ... argument to an integer greater than 1". I assume this is actully the number of threads you want to use, as written it makes it sounds like any interger greater than 1 just turns on the feature. Perhaps we can say "To use multiprocessing, set the threads .... argument to the number of threads you want to use. Setting it to a value of 1 disables the use of threads(? is that right?). Setting it to auto..."

scfleming commented 1 year ago

Towards the end it says "By default multithreading is disabled by default, but you can also disable it by setting threads <= 1". At first I misread this and thought it was backwards but noticed it says "disable it" and not "enable it". If the default is to not enable it, we could just indicate how to enable it (by setting threads > 1) and avoid discussing alternative ways of disabling it to avoid confusing users? The behavior itself is good (that's naturally what someone should expect this to do if given a # of threads of 1 or less) but I don't feel like it's instructive for users to know that in the documentation.

jaymedina commented 1 year ago

If the default is to not enable it, we could just indicate how to enable it (by setting threads > 1) and avoid discussing alternative ways of disabling it to avoid confusing users? The behavior itself is good (that's naturally what someone should expect this to do if given a # of threads of 1 or less) but I don't feel like it's instructive for users to know that in the documentation.

Yep, that's fair, I've removed the comment about how to disable multiprocessing, and just mention that multiprocessing is disabled by default, and updated the second example (without multiprocessing) accordingly.