search

planetarypy / pdsview

Python PDS Image Viewer
BSD 3-Clause "New" or "Revised" License
13 stars 7 forks source link

Issue5 histogram #36

Closed percurnicus closed 7 years ago

percurnicus commented 7 years ago

Histogram Tool

Fixes #5

Create a histogram widget in the main window to adjust the cut sizes.

Histogram Thought Process

The Histogram is a standalone widget that can be used in other applications. It is placed inside a HistogramWdiget to easily include text boxes to adjust the cut levels that way. The HistogramWidget can even be reformatted after subclassing and overriding the create_layout() method to place the text boxes and labels in different locations than below the histogram (see examples). I will move to a more model-view structure in later commits (See Next Steps). The point is, I predict that this widget will be used in other GUIs for planetarypy and I am trying to make it so it is very easy to use it elsewhere. All it needs is an ImageViewCanvas to perform the cuts.

How to Use Histogram:

Examples:

New Layout image

Change Lower Cut image

Change Upper Cut image

RGB for Reference image

RGB with Cuts image

Full Screen image

Additional Notes

This is still a work in progress and not fully tested yet. I wanted to start the pull request in case anyone had thoughts on layout and other features with the histogram that should be added. Also #34 Should be merged before this PR. See Next Steps to see what else is going to be added/addressed and questions on what should be added/addressed.

percurnicus commented 7 years ago

Next Steps

And anything else people think should be addressed or appears when testing

percurnicus commented 7 years ago

CC: @spacerockjock

percurnicus commented 7 years ago

commit https://github.com/planetarypy/pdsview/pull/36/commits/5cfa7a9ea5d7a3f85acea068fc8c0a9e7bf1f540 makes it possible for the user to adjust the number of bins the histogram uses. The default is 100. A question I'm pondering is when the user restores to defaults, should the number of bins restore to 100 too? This is definitely a 'default' but not a default with respect to the image, which the restore defaults button is really referring to. As of right now, restoring does not change the bins and I am leaning toward that answer.

image

image

percurnicus commented 7 years ago

Commit https://github.com/planetarypy/pdsview/pull/36/commits/fda2bb5fd77d7c3a5b29ee908505e52c95882615 makes it impossible for the lower cut to be greater than the higher cut. This is only necessary when manually imputing cuts into the text boxes:

image

Will become (after hitting enter)

image

percurnicus commented 7 years ago

Commit https://github.com/planetarypy/pdsview/pull/36/commits/2cff529a30a4dfdfaed8209d5e1b40ef20d9b7d7 utilizes a model-view structure to implement the histogram. I am planning on making it a MVC structure but need to read more about MVC first

percurnicus commented 7 years ago

Commit https://github.com/planetarypy/pdsview/pull/36/commits/14aa63501dbedb131b4bf2e983f099f5d64e667a creates a warning box that disappears after 3 seconds when erroneous inputs are entered:

image

image

image

percurnicus commented 7 years ago

Commit https://github.com/planetarypy/pdsview/pull/36/commits/a0255c04827061556d02dbb631ce3267032c2e3d does probably too much for one commit but I got carried away. First, I added doc strings to methods and classes. Second, I replaced set_x methods with the proper @x.setter properties to make the code more pythonic and simpler. Lastly, and most importantly, I moved to a MVC structure.

percurnicus commented 7 years ago

Commit https://github.com/planetarypy/pdsview/pull/36/commits/f683e8f31591aedae7be6a5b88dcd4d7918b175c fixes some of the awkward text box sizes

Default Size image

Full Screen image

percurnicus commented 7 years ago

I'm really not sure why the tests are failing. My guess is the anaconda build since I get the same failures on my windows where I use anaconda. Probably going to just revert back to pyside 1.2.0

percurnicus commented 7 years ago

Tests Pass and ready to review @godber and/or @spacerockjock. Still double checking for python3 compatibility. Will double check it works on my windows machine as well. @godber and/or @spacerockjock can either of you check if it works on a mac?

percurnicus commented 7 years ago

Works on Windows

godber commented 7 years ago

I'll try when I can, might be a few days. You should try to update pyside in a future branch probably. You already realized you did more than maybe you should have in this branch.

godber commented 7 years ago

Reverting to PySide 1.2.1 (released in 2013) results in OS X installation issues since there is no precompiled wheel version of that package. I am trying to work around that somehow. At some point you will need to be able to support higher versions.

percurnicus commented 7 years ago

I can go back down to 1.2.0 for this pull request. conda installs 1.2.1 for testing regardless so I thought that would be safe.

godber commented 7 years ago

Don't go lower :) I didn't know you were using conda, that changes things. I was trying to do what was in the README. I will try with conda.

godber commented 7 years ago

If conda is your anticipated deployment environment, I'd make your README reflect that.

percurnicus commented 7 years ago

Well the travis uses conda: .travis.yml#L29. I was using ubuntu and built the wheel for 1.2.4 and 1.2.1 as well as conda on my windows 1.2.1. So I haven't figured out an anticipated deployment envrionment, but I can add a little to the README to explain the installation process as it is right now. I'm planning on doing several pull requests to fix up the documentation, setup/installation process, maybe move to PySide2 and clean up/refactor the code (now that I actually know what I am doing haha). I can update the README and the deployment environment in greater detail then when I've had more time to think about it

godber commented 7 years ago

Maybe you should call OS X broken until you can upgrade to more modern PySide versions (one that has a precompiled wheel) and then you can see where you stand. Even the conda route results in trouble on OS Sierra, conda only includes pyside 1.1.2 for Python 2.7 as far as I can tell.

Maybe start by narrowing down your supported environments, then expanding as you make progress. Adding matplotlib only makes the dependency resolution more challenging, though fortunately there is an OS X wheel for the 1.5.1 release.

Keep an eye on what's available for the versions you choose by looking at them in pypi:

The presence of a wheel typically makes dependency resolution much easier. Obviously the same thing goes for conda.

In fact, I would argue that because your continuous integration tests in travis are using conda ... conda is your primary deployment environment.

I am going to try for a few more minutes, then I am going to have to punt.

godber commented 7 years ago

I think this is confirmation that there is no v1.2.1 conda package for OS X ...

https://anaconda.org/anaconda/pyside/files?version=1.2.1

At least not from the Anaconda team, others may have built it.

Of course, creating a whole conda environment is definitely an option. Albeit heavy handed given your circumstances. Though ... maybe not. Maybe thats the foundation you should lay and build upon. Then at least dependencies would be well constrained across all platforms.

godber commented 7 years ago

FYI, this might be relevant for OS X 10.12 Sierra:

http://stackoverflow.com/questions/41472350/installing-pyside-on-mac-is-there-a-working-method

percurnicus commented 7 years ago

@godber

Seems there is a conda package for 1.1.2 on OS X: https://anaconda.org/anaconda/pyside/files?version=1.1.2 at least...

percurnicus commented 7 years ago

@godber I think any changes to fix/figure out how to best release pdsview should be done in a separate PR and we should consider this PR to be complete.