New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-29893: un-hardcode bps report output #39
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One parameter type comment that should be fixed before merging and one variable name change request that could be done in a later ticket.
python/lsst/ctrl/bps/report.py
Outdated
run_report : `lsst.ctrl.bps.WmsRunReport` | ||
summary : `astropy.tables.Table` | ||
The table representing the run summary. | ||
run_report : `WmsRunReport` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this supposed to have the full name (lsst.ctrl.bps.WmsRunReport)? Others in this file and pull request have full name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should be a full name. I'll fix it.
from lsst.utils import doImport | ||
|
||
from . import WmsStates | ||
|
||
|
||
_LOG = logging.getLogger(__name__) | ||
|
||
SUMMARY_FMT = "{:1} {:>10} {:>3} {:>9} {:10} {:10} {:20} {:20} {:<60}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note that we seem to be losing the columns max widths with these changes. That is fine for this ticket and we'll figure it out in a new ticket if it becomes a problem/nuisance for users.
python/lsst/ctrl/bps/report.py
Outdated
|
||
row = [label] | ||
row.extend([counts[state] for state in WmsStates]) | ||
row.append([by_label_totals[label]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the column name "EXPECTED" better than totals (which sounds just like a sum of other columns). Not necessarily in this ticket, but it would make reading the code easier if the variable for the expected column values was changed as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change to by_label_totals
to by_label_expected
.
The format/layout of bps report was hardcoded. There were hardcoded integers for numbers of hyphens to print and lengths of fields, etc. I rewrote the functions reponsible for displaying the report results with the Table class from 'astropy.table'. As a result, I could get rid of the hardcoded elements of the layout and use formatting capabilities provided by this class.
No description provided.