sot / xija

Thermal modeling framework for Chandra X-ray Observatory
https://sot.github.io/xija
BSD 3-Clause "New" or "Revised" License
9 stars 5 forks source link

Updates for xija_gui_fit #116

Closed jzuhone closed 2 years ago

jzuhone commented 2 years ago

Description

This PR fixes a number of annoying bugs in xija_gui_fit and refactors the design of a few features.

The new look of the window:

Screen Shot 2022-02-03 at 8 51 02 AM

The new "Filters" subwindow, showing how to add a mask and ignore all masks, and how to add a "bad time":

Screen Shot 2022-02-03 at 8 52 05 AM

Testing

jzuhone commented 2 years ago

@taldcroft likely the only FSDS-relevant bits are in https://github.com/sot/xija/pull/116/files#diff-78ad09ad6fe546ab9d4b1d835063650634f425cff3b09df8417a13be438d6838

taldcroft commented 2 years ago

@jzuhone - awesome work glad to see this tool getting continued attention.

I think this will fall in the category of a package update that tags along in a normal Ska FSDS request. It wasn't obvious in 5 seconds what is going on with the diff you highlighted above, but from FSDS perspective the key is to highlight interface impacts at the top level (in the PR description) and make sure in advance that likely users are aware. That pretty much means ACIS ops, @matthewdahmer and occasionally me.

When we make the Ska update request then this can be highlighted if necessary as a notable change.

jzuhone commented 2 years ago

@taldcroft the change in the diff I linked should be a no-op, it's just that it's the only part that could conceivably affect model evaluation if I screwed something up.

taldcroft commented 2 years ago

@jzuhone - I gave this a spin in ska3-next and it works, but I do see a bunch of new warnings:

The process has forked and you cannot use this CoreFoundation functionality safely. You MUST exec().
Break on __THE_PROCESS_HAS_FORKED_AND_YOU_CANNOT_USE_THIS_COREFOUNDATION_FUNCTIONALITY___YOU_MUST_EXEC__() to debug.

@javierggt will probably have a new 2022.2rc4 release out shortly that includes https://github.com/sot/Ska.Matplotlib/pull/24, which changes the default plot_cxctime color (discussed previously on slack). Since this PR won't go in until after 2022.2 (https://github.com/sot/skare3/pull/774) you might as well start testing there.

taldcroft commented 2 years ago

One feature request (and maybe it's already there?) is to be able to get any flight model with the name, e.g.

xija_gui_fit aca --stop=2022:001 --days=200 --set-data='aca0=-10'

This would grab the latest flight model from chandra_models with xija_get_model_spec for any model_name that doesn't end in .json. Or something like that.

jzuhone commented 2 years ago

@taldcroft

when are you getting this warning?

The process has forked and you cannot use this CoreFoundation functionality safely. You MUST exec().
Break on __THE_PROCESS_HAS_FORKED_AND_YOU_CANNOT_USE_THIS_COREFOUNDATION_FUNCTIONALITY___YOU_MUST_EXEC__() to debug.

I don't see it.

taldcroft commented 2 years ago

Huh, I tried again and I don't see that now. Well, never mind for the moment.

matthewdahmer commented 2 years ago

@taldcroft @jzuhone I saw that same error, "__THE_PROCESS_HAS_FORKED...", once on my Apple Silicon Mac. During that instance, once I saw it I was not able to get xija_gui_fit to run until I rebooted, then it worked fine.

jzuhone commented 2 years ago

Weird. Anyway, I added the new feature @taldcroft requested. It also prompts if you have an unsaved model when you hit the Quit button.

taldcroft commented 2 years ago

I added the new feature @taldcroft requested. It also prompts if you have an unsaved model when you hit the Quit button. Nice! I also like the new Thaw/Freeze boxes, when I was using the previous version I had to think for a few seconds to remember the right words.

One more feature request - put the xija version into the App title? In testing just now I had been accidentally getting the wrong version so it would be nice to see the version.

jzuhone commented 2 years ago

Ok to merge this before things get too busy?

taldcroft commented 2 years ago

Fine by me to merge, but @matthewdahmer - are you good with this?

matthewdahmer commented 2 years ago

@jzuhone @taldcroft I am OK with this update. I tested it on my Mac with a PLINE refit and seems to work well!

taldcroft commented 2 years ago

BTW @jzuhone have you tested this with ska3-next on supported platforms?

I guess that isn't a lien on merging this since we can always come back with fixes as needed. My light testing was OK on that but it's worth noting that @javierggt experienced troubles with aca_view (another Qt app) and required export QT_MAC_WANTS_LAYER=1 to get it to work. (It is slightly perplexing that xija_gui_fit works without that).

jzuhone commented 2 years ago

@taldcroft I have not yet tested against ska3-next--is there a documented way yet to set up a ska3-next environment?

taldcroft commented 2 years ago

@jzuhone - the conda command under Testing in https://github.com/sot/skare3/pull/774 should work.

jzuhone commented 2 years ago

@taldcroft I tested under ska3-next and all appears to work fine.

taldcroft commented 2 years ago

👍

jzuhone commented 2 years ago

@taldcroft what's your position on who should merge this?