Fixes in supplement update script (start/stop, input/output, docs) #54
Conversation
4368400 to
416f6d8
Compare
In new code I always make the default be "safe" for any output files that get written. I.e. if you just run the script without arguments then nothing can go wrong with the flight files. How you do that depends a bit on the app, but the upshot is that the output "flight" values need to go into |
|
yes, that makes sense. I will do that instead (default |
agasc/scripts/mag_estimate_report.py
Outdated
| @@ -19,7 +20,9 @@ def parser(): | |||
| ' CxoTime-compatible time stamp.') | |||
| parse.add_argument('--stop', help='Time to stop processing new observations.' | |||
| ' CxoTime-compatible time stamp.') | |||
| parse.add_argument('--out-dir', default=f'./mag_stats_report', | |||
| parse.add_argument('--input-dir', default='$SKA/data/agasc', | |||
| help='Directory containing mag-stats files') | |||
There was a problem hiding this comment.
It's generally good to include the default values in the help string. (It seems like argparse should be able to do that automatically, but I don't recall this functionality).
There was a problem hiding this comment.
Good point. I will do that. For some reason, argparse does not do it. The sphinx-argparse module does, so it shows in web docs, but not in the command-line help.
agasc/scripts/mag_estimate_report.py
Outdated
| parse.add_argument('--out-dir', default=f'./mag_stats_report', | ||
| parse.add_argument('--input-dir', default='$SKA/data/agasc', | ||
| help='Directory containing mag-stats files') | ||
| parse.add_argument('--output-dir', default=f'$SKA/www/agasc_supplement_reports/suspect', |
There was a problem hiding this comment.
This path seems weird. I would expect more like $SKA/www/ASPECT/agasc/agasc_supplement_reports/suspect, or something like that.
Also (though this not set in stone), I would usually define a configurable root directory like $SKA/www/agasc and then have the rest agasc_supplement_reports/suspect hardcoded. I.e. only define root directories, not detail directories down lower.
|
@javierggt - it looks like you agreed to the point about not writing into flight dirs by default but the code still does that. About the output directory, we need to be a little careful about the actual disk that files are being written to. The "originals" should stay on /proj/sot, whereas /proj/sot/ska/www/ASPECT is actually /data/mta4/www/ASPECT (not controlled by us). My first instinct is to write the reports as a sub-directory of /proj/sot/ska/data/agasc so they are more easily discoverable (by me at least) and we can deal with disk space issues if they arise. @jeanconn did the same for the dark cals, and demonstrated that a soft link from the /data/mta4 filesystem back to /proj/sot seems to work for the web server (e.g. https://cxc.cfa.harvard.edu/mta/ASPECT/acdc/). |
|
Regarding being "safe" in where outputs go in new code... I've been somewhat casual in my new code if the code is just reporting/trending and doing nothing that is really operational. For this code that will have an operational component, I agree with the plan of having it not write into flight areas by default. |
|
I made the change so reports are not saved in /proj/sot/ska/www/ASPECT |
…nsible defaults, and using pathlib instead of os.path
…ly run. Added --whole-history argument.
0886627 to
35fdb5d
Compare
agasc/scripts/mag_estimate_report.py
Outdated
| ' Default: now') | ||
| parse.add_argument('--input-dir', default='$SKA/data/agasc', | ||
| help='Directory containing mag-stats files. Default: $SKA/data/agasc') | ||
| parse.add_argument('--output-dir', default='$SKA/www/ASPECT/agasc/supplement_reports/suspect', |
There was a problem hiding this comment.
Still outputting to a flight directory.
There was a problem hiding this comment.
oops! It looks like I am actively trying to pass it under the rug...
Fixed it.
Description
This PR introduces a few changes, one of which is a bug fix:
Testing
Fixes #