nanograv / PINT

PINT is not TEMPO3 -- Software for high-precision pulsar timing
Other
121 stars 101 forks source link

pintk feature requests #1203

Open scottransom opened 2 years ago

scottransom commented 2 years ago

After working on pintk a bunch over the last couple weeks, there are a bunch of features that I've thought of and think we should implement. So I'll start this list and encourage people to add other ideas in the comments (and I'll update the list).

Stuff listed with strikethrough has already been completed

almcewen0 commented 2 years ago

C&P from discussion on slack, responses from @scottransom :

selecting points is handy, but how do i deselect the most recent chunk? ‘u’ always deselects everything. Can't do that now, but that should be doable. In fact, that used to be possible, but I removed the functionality which depended on deprecated code in toa.py. Is this really useful, though? I haven't really seen a good use case for it.

well sometimes I accidentally select some TOAs I don't want to, and if I am choosing a few groups, it's annoying to have to start over when i screw up.

if I uncheck the components at the top, i want to deselect all the underlying parameters (otherwise i accidentally fit for parameters i don’t mean to) That's a good point.

why hide those components at all? Also a good point.

don’t reset the zoom when i fit I'm not sure I agree on this one. When phase connecting, the scale can change hugely. Why not reset?

sure, but if I am fitting a small group of the TOAs and omitting the others, I like to be zoomed on those to be sure they look good after the fit. not a huge deal, but just the routine I am used to 😅

add new fit parameters (i.e. higher order frequency derivatives) quickly Not sure what you mean here. Do you mean add more at a time? If so, how many more?

I mean add them to the model. so if i have F0 and need F2, but it isn't in the parfile, it would be great if I could add it with the GUI.

i use “INCLUDE” statements in my tim files, so i have one file with several lines pointing to a number of other files. might be nice to include a dropdown where i can select/deselect those files within the GUI That would be tricky, I think. But might be doable in other ways...

when i revert, reset the zoom as well Not sure what you mean. I think you get the zoom level that you had previously. Is that not what you want?

that is what i want, but that wasn't the behavior i was seeing. if i zoom in on a chunk, select it, fit, then revert, it seems like it resets the zoom in the y axis, but not along the x axis. weird!

option to select all TOAs in the current window (i.e. after zooming) That's a good idea. Not sure how easy it will be to implement, but I'll try to. That seems definitely useful.

‘k’ is good, but it would be better if it would “korrect” the pane to show all the points in a zoomed window, not zoom back out to show everything (that is also useful, but sometimes i misclick) Do you mean just optimize the zoom for the points you've selected within the zoom? One issue is that zooming does not select points (on purpose)!

yep, that's what i mean. if i am zoomed into a block and hit 'k', it would be nice if it just scaled the zoom so that all the TOAs in the MJD range i'm looking at are visible

option to select/deselect all fit parameters That should be easy. Good idea.

“JUMPed” coloring doesn’t always work, i was confused about why half my TOAs suddenly turned red Yup. I've seen that once or twice. Not sure I can replicate it properly, though. If you figure out how, please let me know!

it seems like the TOAs that are JUMPed turn red when fitting is turned off for the JUMP. not sure if that's intended

why can’t i delete JUMPed TOAs? Delete the TOAs? Or the JUMPs? Or (some) TOAs within a JUMP? The big thing is that you can't delete all TOAs within a JUMP or you need to delete the JUMP first. But I think you should be able to delete some TOAs within a JUMP. Might take some effort.

actually, it seems like this might be related to the previous point - i get an error when i try to delete a point that is in a JUMP, but only if i turn of the fit for JUMP1 first.

scottransom commented 2 years ago

For the comment about F0 in the parfile and wanting to add F2. You can already do that with my new changes. But you just need to add F1 first. And then fit. Then F2 will pop up. So you should always have the next non-fit spin-freq deriv ready to go.

As for colors of JUMP'd TOAs. If you are using the default color scheme. JUMP'd TOAs (fit or otherwise) are red. Selected TOAs are yellow. And all other TOAs are blue. So you should see your JUMP'd TOAs being red. Is that what you are seeing? I'm happy to change that if we think something different would be more useful.

almcewen0 commented 2 years ago

hm, i am not getting the higher order derivatives. i have F1 and F0, but would like F2 - to get it, i had to edit the par file. same deal with F3, which i just tried as well.

that's not quite what i see. the only time the TOAs turn red is if they are included in a JUMP and the fitting for the JUMP is turned off. when it is on, all unselected TOAs are blue.

almcewen0 commented 2 years ago

also (and this might be a bit too much for your every-day GUI), it would be cool to be able to add in a binary component from the GUI. even cooler if it could try to estimate the binary parameters from measured Fdots.

scottransom commented 2 years ago

Did you do a get pull to get the PR that was just merged a few hours ago? Because that is the one that fixed the "adding freq derivatives" thing.

scottransom commented 2 years ago

And I think that same PR fixed an issue with the JUMP coloring as well.

almcewen0 commented 2 years ago

aha, i had not! thanks.

dlakaplan commented 2 years ago

Suggestions:

dlakaplan commented 2 years ago

Another question: how is the fitter type determined? Would pintk know to use a WB fitter? Could we make the fitter selectable on the command line and/or with a dropdown menu?

scottransom commented 2 years ago

So I just added the "q" and "s" key suggestions. And I agree that we should show the chi^2 of the selected points from the fit. I'll make that change. Do you think that showing the chi^2 of all the TOAs is useful at all? Or should I just skip that?

dlakaplan commented 2 years ago

Great. I've added version info and a fitter-selection dropdown. I'm working to make sure that it doesn't allow Wideband fitters with NB data and vice-versa.

Not sure about showing the chi^2 of all of the data. It probably is useful, if only to get the chi^2 of the data without a refit somehow. Maybe just make that a separate action?

scottransom commented 2 years ago

Excellent. I don't think I can do any more development today, but I'll definitely fix the chi^2 thing before doing the video demo. And I have pushed up my latest changes, so hopefully you don't get any conflicts.

dlakaplan commented 2 years ago

OK, I:

dlakaplan commented 2 years ago

Oh, looks like your latest push did what my last one did.

dlakaplan commented 2 years ago

@scottransom : see https://github.com/nanograv/PINT/pull/1216/

swiggumj commented 2 years ago

Issues I'm running into...

scottransom commented 2 years ago

@swiggumj That first issue is strange! What are you seeing when you first open pintk? There is nothing at all in the window? As for the "refresh" function deprecation, that's interesting. I'm using Matplotlib 3.5.1 and don't think that I've ever seen it. And finally, there is also the regular Tk visual clue about zoom mode -- the zoom button is either pressed or not. I guess we could see about something more visually striking, though.

dlakaplan commented 2 years ago

There are various deprecation warnings that some people see/don't, depending on their exact setups. There are also the ERFA warnings which people are seeing. It's possible to make both go away but we should decide which/how (just disappear, or appear once then disappear).

paulray commented 2 years ago

When I Re-fit, the Color mode radio button goes back to "default" but the color mode I've selected stays active.

dlakaplan commented 2 years ago

I occasionally forget the order of the par and tim files. Should we test the parsing both ways and swap if one fails? Or allow command-line arguments like --par=.... --tim=...?

dlakaplan commented 2 years ago

Printing out the full par file at the top may be too noisy, especially for a complex file that has lots of jumps/noise parameters. We could tie that to the logging level (or even do it via log.info) or somehow filter the output?

swiggumj commented 2 years ago

@scottransom I see everything except the plot (see attachment). I also meant to note above that when I do a fit, I have to refresh the plot before the results show up (e.g. hit the home button). David said he hasn't encountered that, but said it might have something to do with some window management subtlety.

Screen Shot 2022-05-03 at 10 00 48 AM
swiggumj commented 2 years ago

Another issue I'm running into is a bit complicated and has to do with adding/removing jumps to find a coherent timing solution for a binary MSP. Maybe it's better to discuss on an upcoming call, but I can successfully add jumps for N-1 epochs and fit for F0, PB, A1, TASC (for example) and get flat residuals. I've had difficulty removing these jumps, however, to build phase connection over longer periods of time and start fitting for, e.g., RA, DEC, F1.

Part of the issue seems to be with the way that jumps are labeled in both the par/tim files, -gui_jump 1. If I save the resulting par/tim file from a fit where I get flat residuals, I can remove jumps in the par file, but there is no straightforward way of doing this with corresponding jump flags in the tim file (that I know of...?). Am I missing something? Also, the lingering jump flags in the tim file can be problematic if I try to re-add jumps later on since the name is so generic (again, -gui_jump 1). It's very possible I'm doing something wrong or sub-optimally and if so, please let me know!

scottransom commented 2 years ago

@swiggumj Thanks for the additional explanation about the plotting issue! That's very strange that the plot frame doesn't even show up. Huh.

And I 100% agree with the JUMPs issue. Removing jumps is really tricky now and it is something we definitely need to fix soon. I think that could potentially be a good project for a student/postdoc who wants to get started in PINT stuff. Or since I'll be spending 40+ hours on airplanes over the next 2 weeks, maybe I can look into it then! :-|

dlakaplan commented 2 years ago

Not that it's important, but I added an icon. I don't know how well this works on different OSs. Is anybody able to try https://github.com/dlakaplan/PINT/tree/pintktweak

dlakaplan commented 2 years ago

@swiggumj : would something like this help with figuring out which mode (zoom or select) is current?

Screen Shot 2022-05-06 at 12 07 25 PM Screen Shot 2022-05-06 at 12 07 30 PM
swiggumj commented 2 years ago

Oooo yeah, I like that!

dlakaplan commented 2 years ago

Currently that is implemented in https://github.com/dlakaplan/PINT/tree/addzoomstatus. But I won't do a PR until we discuss further.

dlakaplan commented 2 years ago

In that same fork I added chi^2 of just the selected points. @scottransom : hopefully I did this correctly. Usage comes either with the "x" key (didn't seem to be used) or as part of the info when you hit space. Example:

Selected TOAs:                 89
Selected Chi2:          4263.1393
Selected DOF:                  86
Selected Reduced-Chi2:  49.571387
Selected Weighted RMS:  14934.881 us
------------------------------------
Selected TOAs:                 16
Selected Chi2:          16.799302
Selected DOF:                  13
Selected Reduced-Chi2:  1.292254
Selected Weighted RMS:  1997.41 us
------------------------------------
Selected TOAs:                  4
Selected Chi2:          18.403031
Selected DOF:                   1
Selected Reduced-Chi2:  18.403031
Selected Weighted RMS:  8283.4274 us
------------------------------------

those are all without refitting. Perhaps the reduced chi^2 shouldn't be used, since the number of DOF isn't consistent. But I thought it was helpful to add this (e.g., select a group of points, use this to see if an EFAC or EQUAD is needed).

bwmeyers commented 2 years ago

Issue I just ran into when trying this out on some CHIME data with a tempo2 generated parfile: the START and FINISH keys are set and, for whatever reason, are followed by 1 0.0, e.g.,

START              59030.5932034769185710 1 0.0
FINISH             59328.7824288187154490 1 0.0

and I think this makes PINT try to "fit" them, or at least it does in pintk. I get the following error -

Traceback (most recent call last):
  File "/usr/lib/python3.8/tkinter/__init__.py", line 1892, in __call__
    return self.func(*args)
  File "/home/bmeyers/gbncc/venv/lib/python3.8/site-packages/pint/pintk/plk.py", line 459, in fit
    self.fit_callback()
  File "/home/bmeyers/gbncc/venv/lib/python3.8/site-packages/pint/pintk/plk.py", line 664, in fit
    self.psr.fit(self.selected)
  File "/home/bmeyers/gbncc/venv/lib/python3.8/site-packages/pint/pintk/pulsar.py", line 372, in fit
    fitter.fit_toas(maxiter=1)
  File "/home/bmeyers/gbncc/venv/lib/python3.8/site-packages/pint/fitter.py", line 1933, in fit_toas
    M, params, units = self.get_designmatrix()
  File "/home/bmeyers/gbncc/venv/lib/python3.8/site-packages/pint/fitter.py", line 583, in get_designmatrix
    return self.model.designmatrix(toas=self.toas, incfrozen=False, incoffset=True)
  File "/home/bmeyers/gbncc/venv/lib/python3.8/site-packages/pint/models/timing_model.py", line 1729, in designmatrix
    q = -self.d_phase_d_param(toas, delay, param)
  File "/home/bmeyers/gbncc/venv/lib/python3.8/site-packages/pint/models/timing_model.py", line 1565, in d_phase_d_param
    d_delay_d_p = self.d_delay_d_param(toas, param)
  File "/home/bmeyers/gbncc/venv/lib/python3.8/site-packages/pint/models/timing_model.py", line 1578, in d_delay_d_param
    raise AttributeError(
AttributeError: Derivative function for 'START' is not provided or not registered.

Which is fine, because it's obviously not supposed to be including START/FINISH in the timing model fit, but I feel like those should maybe be ignored or fixed and have a WARNING message, rather than cause the whole process to fail?

dlakaplan commented 2 years ago

So the parfile was from tempo2? I think in that case this is a separate issue (i.e., general pint, not pintk). it doesn't matter too much, but if you file it separately it will be easier to track. If you include the tim/par files for testing we can be sure to fix it.

dlakaplan commented 2 years ago

Add "color by jump" mode, perhaps with some sort of JUMP browser.

dlakaplan commented 2 years ago

Add stand-alone documentation

dlakaplan commented 2 years ago

https://github.com/nanograv/PINT/pull/1259 does the "color by jump", although it doesn't do any other interface

aarchiba commented 2 years ago
* Writing of new .tim files shouldn't lose any information (if possible) from original TOAs (the T2-format names change now)

Can you provide an example where this is true? If I load a t2-format tim file, the t2 names go in -name, and -name goes into the t2-format name on write.

almcewen0 commented 2 years ago

a few more possible upgrades:

  1. is there any way to add to the stashed TOAs? it would be nice if i didn't have to unstash/restash everything first.
  2. i find it a little annoying that the different parameters get hidden under the first checkbox at the top. just show them all, always (or have an option to do so).
  3. it would be nice if i could update the model that pintk will "reset" to. so if i start from scratch and connect a few days together reliably, i might like to "checkpoint" that model so that if things go nuts i can go back to it by hitting reset without having to save a new model and restart pintk