Skip to content
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

Compare parfiles #1340

Merged
merged 16 commits into from
Jul 14, 2022
Merged

Compare parfiles #1340

merged 16 commits into from
Jul 14, 2022

Conversation

dlakaplan
Copy link
Contributor

  • Command-line utility to compare par files
  • Use colors for output
  • Bugfixes to reduce crashes

@dlakaplan
Copy link
Contributor Author

Usage:

(pintdev) kaplan@plock[~/pythonpackages/PINT_dlakaplan] (comparepar) % compare_parfiles -h                         
usage: compare_parfiles [-h] [--dmx] [--no-dmx] [--sigma SIGMA] [--uncertainty_ratio UNCERTAINTY_RATIO] [--nocolor]
                        [--comparison {max,med,min,check}] [--log-level {TRACE,DEBUG,INFO,WARNING,ERROR}]
                        input1 input2

PINT tool to compare parfiles

positional arguments:
  input1                First input parfile
  input2                Second input parfile

optional arguments:
  -h, --help            show this help message and exit
  --dmx                 Print DMX parameters (default: False)
  --no-dmx              Do not print DMX parameters (default: False)
  --sigma SIGMA         Pulsar parameters for which diff_sigma > threshold will be printed with an exclamation point at the end of the line (default:
                        3)
  --uncertainty_ratio UNCERTAINTY_RATIO
                        Pulsar parameters for which the uncertainty has increased by a factor of unc_rat_threshold will be printed with an asterisk
                        at the end of the line (default: 1.05)
  --nocolor             Turn off colorized output (default: False)
  --comparison {max,med,min,check}
                        "max" - print all lines from both models whether they are fit or not (note that nodmx will override this); "med" - only print
                        lines for parameters that are fit; "min" - only print lines for fit parameters for which diff_sigma > threshold; "check" -
                        only print significant changes with logging.warning, not as string (note that all other modes will still print this)
                        (default: max)
  --log-level {TRACE,DEBUG,INFO,WARNING,ERROR}
                        Logging level (default: WARNING)

@dlakaplan
Copy link
Contributor Author

@scottransom : I am trying to colorize the output. But I want to just adjust the background color while leaving the foreground static. is there a way to do that?

@scottransom
Copy link
Member

Hmmm. Good question. I think to do that you would somehow have to query your terminal environment to know what the current foreground and background colors are. I certainly don't know of a way to do that.

@aarchiba
Copy link
Contributor

aarchiba commented Jul 13, 2022

Would it be possible to (optionally?) output a Markdown table, where markdown highlights (*...* or /.../ say) were used to indicate things of interest? A Markdown table is quite a clear ASCII representation but also allows a web browser to render a nicer version. In particular there are libraries to handle displaying Markdown nicely (with colours!) on a terminal: https://python.plainenglish.io/dump-the-plain-text-help-in-your-command-line-interfaces-today-3282225274dd

(ETA: And notebooks, of course, can natively display Markdown output with a very little persuasion.)

@dlakaplan
Copy link
Contributor Author

I was part-way to turning it into more table-like output anyway, so if I finished that adding markdown would be trivial.

I did search on how to find the current foreground/background colors, but couldn't find a good answer. This is mostly relevant since e.g., I have white text on black as the default, so just fg=black isn't right.

@codecov
Copy link

codecov bot commented Jul 13, 2022

Codecov Report

Merging #1340 (3039866) into master (24e144a) will decrease coverage by 0.06%.
The diff coverage is 59.82%.

@@            Coverage Diff             @@
##           master    #1340      +/-   ##
==========================================
- Coverage   61.31%   61.25%   -0.07%     
==========================================
  Files          89       90       +1     
  Lines       20218    20349     +131     
  Branches     3619     3646      +27     
==========================================
+ Hits        12397    12465      +68     
- Misses       7046     7097      +51     
- Partials      775      787      +12     
Impacted Files Coverage Δ
src/pint/scripts/compare_parfiles.py 0.00% <0.00%> (ø)
src/pint/models/timing_model.py 81.63% <67.85%> (-1.77%) ⬇️
src/pint/utils.py 57.69% <100.00%> (+1.41%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24e144a...3039866. Read the comment docs.

@dlakaplan
Copy link
Contributor Author

Here is what the markdown output looks like:
Screen Shot 2022-07-13 at 2 40 36 PM

@dlakaplan
Copy link
Contributor Author

@almcewen0 : since you worked on this some, maybe you can take a look. Especially if you use this in notebooks see how the markdown output looks

@almcewen0
Copy link
Contributor

i think it looks great on the command line! it doesn't run in a jupyter notebook, but that might be better suited for the model.compare() function.

@dlakaplan
Copy link
Contributor Author

The command-line version wraps model.compare(). it's changes to that function which do markdown in a notebook

@dlakaplan
Copy link
Contributor Author

@swiggumj : would this markdown output be using in timing analysis?

@swiggumj
Copy link
Contributor

This looks great. Yeah, I think so!... I'd have to check, but I think it also might help clean up some of the code where things get compiled into per-pulsar summary files.

@dlakaplan
Copy link
Contributor Author

@aarchiba
Copy link
Contributor

This looks great. Yeah, I think so!... I'd have to check, but I think it also might help clean up some of the code where things get compiled into per-pulsar summary files.

Indeed. This could (should!) be used directly in the timing_analysis notebooks so you can show the comparison. For the summaries, right now the table is shown as preformatted, but this would allow us to produce much nicer-looking tables as a drop-in replacement.

@dlakaplan
Copy link
Contributor Author

Were you thinking that the fit.get_summary() method should also return markdown? Possible - I just didn't know if it would be useful. But I can add it.

@scottransom
Copy link
Member

That looks sweet. But looking at the readthedocs example, what is up with F1?!

@dlakaplan
Copy link
Contributor Author

No idea - it's also in the cells above when the Powell fit results are shown. And I think that has been around for a while:
https://nanograv-pint.readthedocs.io/en/latest/examples/understanding_fitters.html#Powell-fitter
So I can investigate but probably a separate issue.

@dlakaplan
Copy link
Contributor Author

@scottransom : actually wait a second. I want to update the docs slightly.

@scottransom scottransom merged commit bdf1c67 into nanograv:master Jul 14, 2022
@scottransom
Copy link
Member

Oops! I saw that too late!

@dlakaplan
Copy link
Contributor Author

OK, I guess I was too late. I will just edit that directly then (just to note that without raw=True the markdown in notebooks will vanish)

@scottransom
Copy link
Member

I think you can just merge the doc changes directly in since they don't affect our CI tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants