Log readable step pars #140
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #140 +/- ##
==========================================
+ Coverage 59.25% 59.33% +0.07%
==========================================
Files 24 24
Lines 1615 1618 +3
==========================================
+ Hits 957 960 +3
Misses 658 658 ☔ View full report in Codecov by Sentry. |
|
The resulting log when running JWST |
jdavies-st
force-pushed
the
log-readable-step-pars
branch
from
bc3b861 to
b191918
Compare
|
This looks readable now. Perhaps print pre_hooks and post_hooks only if they are not empty (I wonder if the check will noticeably slow things down, probably not). Otherwise it's very repetitive. |
|
Much needed in my opinion. |
|
Looks good. Some of it does get a bit repetitive (all the params that are common to all steps), but not sure how we could about intelligently fixing that. |
|
Yeah, it's very repetitive on all the common Step parameters, including pre- and post-hooks, and including We could conditionally print them out if they are different from the default? Thoughts? That said, it might be worth revisiting what things should be user-settable via For example, So worth revisiting in a separate issue or PR, but perhaps not here. |
jdavies-st
force-pushed
the
log-readable-step-pars
branch
from
b191918 to
e3c8fba
Compare
👍 |
IMHO, I'm not sure that's a good thing, especially for steps like jump or tweakreg or resample, where there are lots of keywords normally set to defaults, but those defaults are not something benign like I agree that we could potentially omit ones like |
|
How about we decide what in There's a couple downstream test failures in |
|
Making tweaks in another PR is fine with me. Let's at least get the major formatting change from this ticket merged. |
|
One question/suggestion/comment. If outputting the config in yaml, any reason to not keep |
Only the written ASDF config file actually contains the yaml. But generally one shouldn't be cut-and-pasting to make these, and there's a method for writing ASDF config files out already. If one wants to cut-and-paste, it's usually in a python script or notebook, so as you point out it's good to have python types. And the |
|
I haven't looked at the code, but is it hard to keep a stack of the parameters, and when printing just print ones have changed relative to the level above? |
|
@perrygreenfield parameters are always set at the beginning of a step or pipeline (which has steps) and don't change, but pipelines have steps and each step can set all the standard parameters. Most are not set differently from default, but occasionally they are. Longer log, but perhaps more readable. On balance, because we are not repetitively printing out the parameters at each pipeline step (just the whole pipeline at the beginning), the logs will actually be a bit shorter. Agree that it would be nice to give users useful information instead of repetitive, content-free information. We can think about improvements in a future PR. Btw, I added a commit to add back the Excluding it was causing 3 downstream tests in |
|
spacetelescope/jwst#8317 should be merged before this is merged. This fixes the failing downstream test in |
jdavies-st
force-pushed
the
log-readable-step-pars
branch
from
7200a6a to
e747142
Compare
|
The one downstream test fix in |
|
JWST regtests running: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1268/ |
|
romancal regtests showed no errors I believe the above errors are unrelated to the logging changes in this PR. @schlafly would you like to weigh in as this will impact romancal? |
|
I'm happy. @ddavis-stsci , do you want to weigh in here? |
|
I'm fine with this. I certainly makes the output more readable! |
This PR makes the logging of Step or Pipeline parameters legible.
True,FalseandNoneinstead of YAML types true, false and null.This change depends on
pyyaml, but we already implicitly depend on it viaasdfwhich pulls it in. Still the dependency is made explicit here in pyproject.toml.Plus a small context manager cleanup in a very old test that uses
StringIO. In Python 2, this did not have a context manager. It does in Python 3, so use it.Resolves #137