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

*working* qt port of gui_fit #56

Closed jzuhone closed 5 years ago

jzuhone commented 5 years ago

This PR implements a working PyQt5 port of gui_fit. As far as I can tell, it implements all of the features from the previous gtk version. I have been testing it in ska3 (haven't tried getting it to work in ska2, not sure if we want to). This is based originally off of @taldcroft's original work.

Some other things I did here:

Feel free to offer suggestions as to other ways to test this, or changes. It is going to be pretty difficult to evaluate this from code diffs, so probably just trying to run it will be the best way to do so.

Tests

Do these for officially supported platforms (linux-64 CentOS-6, 7 and MacOS High Sierra (Mojave?)).

ping @jeanconn @richardjedgar @Gregg140 @matthewdahmer

taldcroft commented 5 years ago

Nice!! :tada:

jzuhone commented 5 years ago

screenshot from running this on my Mac:

screen shot 2018-11-08 at 1 40 35 pm

Gregg140 commented 5 years ago

I gave this the basic smoke tests - ran gui_fit, tested all the control icons (e.g. pand zoom), moved the sliders and did a quick fit.

It all seems to be working quite well.

Well Done!

richardjedgar commented 5 years ago

This looks great! It seems to have all the functionality of the old version (plus the needed enhancement of normalizing the red/blue colors to match the ones in acis thermal check).

jzuhone commented 5 years ago

@taldcroft I think from the ACIS side this is ready to go, so whenever you have time to look at it let me know.

taldcroft commented 5 years ago

This is embarrassing, but I seem to not be able to build xija now on HEAD linux. This is on master on kadi (CentOS-6). On CentOS-5 (coot) it builds just fine and passes tests. @jzuhone @jeanconn any ideas? What was your build process and testing on linux?

ska3-kadi$ python setup.py test
running test
running egg_info
writing xija.egg-info/PKG-INFO
writing dependency_links to xija.egg-info/dependency_links.txt
writing top-level names to xija.egg-info/top_level.txt
reading manifest file 'xija.egg-info/SOURCES.txt'
writing manifest file 'xija.egg-info/SOURCES.txt'
running build_ext
building 'xija.core' extension
creating build
creating build/temp.linux-x86_64-3.6
creating build/temp.linux-x86_64-3.6/xija
gcc -pthread -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -fPIC -I/proj/sot/ska3/flight/include/python3.6m -c xija/core.c -o build/temp.linux-x86_64-3.6/xija/core.o
In file included from /proj/sot/ska3/flight/include/python3.6m/Python.h:30:0,
                 from xija/core.c:3:
/usr/include/string.h:548:5: error: unknown type name \u2018__locale_t\u2019
     __locale_t __loc)
     ^
/usr/include/string.h:552:18: error: unknown type name \u2018__locale_t\u2019
      size_t __n, __locale_t __loc)
                  ^
error: command 'gcc' failed with exit status 1
taldcroft commented 5 years ago

I note that /usr/include/string.h has a date of June 19, 2018 on kadi, and it conceivable I haven't built kadi from scratch since then (probably instead relying on the already-built library in my git dir).

taldcroft commented 5 years ago

I did get your branch working with the CentOS-5-built core.c, looks nice. But didn't really yet test it

jeanconn commented 5 years ago

With regard to xija building on kadi from ska3, we should probably open this issue in a skare3 thread. I had not been doing any building with ska3/centos6 because that isn't our build platform. It looks like it is broken. skare3 is installing a gcc 4.8.5 in $SKA/bin/gcc and my best guess is there's a conflict on CentOS6 when using it and CentOS6 system includes to build xija core.so.

Since, of course, the dream is to eventually build on CentSO6 or 7, it does make sense to build test on it and figure this out, but we may be able to move to a modern gcc at that time or reliably use the system gcc.

jzuhone commented 5 years ago

I have never had any problems building xija on any CentOS6 or 7 machine, but I also never used kadi.

jeanconn commented 5 years ago

I couldn't build from fido (also CentOS6) in a ska3 environment either, so I figured not specific to kadi.

taldcroft commented 5 years ago

Which machines did you build on? Did you delete the build dir first? I had also tried fido last night.

taldcroft commented 5 years ago

@jzuhone - I added some tests to the description. You can check off there and (if relevant) provide supporting test documentation in a separate comment. Some tests are trivial, but the idea is to document that you tested everything the GUI interface is supposed to do.

taldcroft commented 5 years ago

Since this will become a global symbol in the path, I would suggest renaming to something like xija_gui_fit to make it more specific. That would partner with xija_fit.

Silly question, but within a development (git) directory, what's the easiest way to test an entry-point script? I ended up updating PYTHONPATH and installing to a local dir, but there must be a better way. I guess python -m xija.gui_fit:main should work? (But I never remember if there is a space after the -m or not).

taldcroft commented 5 years ago

One minor issue I notice on linux was that the initially thawed (fitted) parameters were shown with a solid filled box. Clicking the box would make it empty, but then clicking again gives a check in the box, not a solid box. Not a big problem if it works, but still a little disconcerting / inconsistent.

jzuhone commented 5 years ago

@taldcroft I built on barth-v, which is an ACIS virtual machine that you can try if you like and it's a CentOS7 machine.

taldcroft commented 5 years ago

Some testing on Mac. Overall really awesome. Here are some issues that I don't consider stoppers for merging this, and these may even be in the legacy Gtk version.

I did the following testing on my iMac with the DPA model:

jzuhone commented 5 years ago

@taldcroft sounds good--I think I can solve the first two issues you mentioned easily, will have to look more carefully at the second one.

I've worked through about half of the tests on CentOS 7 so far.

jzuhone commented 5 years ago

@taldcroft so I can reproduce the Stop button hang on macOS but it doesn't happen on Linux. I'm worried that there is some dependence on how multiprocessing is handled in the two different OSes (which I know hardly anything about).

taldcroft commented 5 years ago

@jzuhone - annoying but not a blocker.

jzuhone commented 5 years ago

@taldcroft I did not necessarily do a reproduction of the current model from the old model for the DEA, but I did take an old model and re-fit it with recent data and got a better fit from the dashboard plot perspective. should that be sufficient?

jzuhone commented 5 years ago

@taldcroft "better" as in better than the current model

jzuhone commented 5 years ago

Hi @taldcroft. Sorry to bother you while away, but is there anything else we need to do here?

taldcroft commented 5 years ago

There's one test box not checked. Thoughts on that test?

jzuhone commented 5 years ago

@taldcroft I made a comment above about this, but forgot to check the box. Here's what I actually did: I took the current DEA model and went through the entire process of refitting and got a better fit from the dashboard plot perspective. should that be sufficient?

taldcroft commented 5 years ago

@jzuhone - fantastic. I just thought of one last thing which is about the test environment. Did you test from an installed version or from within git? Basically install to a test env to make sure the setup.py and entry point is correct.

@jeanconn - once @jzuhone confirms the install test this is good to merge and put into ska3-flight candidate and bring to LR for review with the rest.

jzuhone commented 5 years ago

@taldcroft @jeanconn I ran this in a local Ska3 test environment and I installed the package via python setup.py develop and then used the command-line script it made via the entry point to actually run it (as well as @richardedgar and @gregg140, who successfully sourced and ran from my environment on their own boxes), so I think I pass this test also.

jeanconn commented 5 years ago

Does this get cut as a xija 4.13 release?

taldcroft commented 5 years ago

Yes (unless I'm missing something...).

jeanconn commented 5 years ago

@jzuhone marked it as 3.13, but I figured if this is ska3 only should increment major version.

jzuhone commented 5 years ago

Whatever you decide is fine by me.