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

Cleaning UI and improving positioning of Panel2D #1441

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@MarcCote
Contributor

MarcCote commented Mar 4, 2018

This PR exists to reduce changes in #1355.

Remove unused attributes in UI.
Add a better way of handling absolute/relative coordinates in Panel2D.
Remove FileSelectMenu2D that will be replaced with ListBox2D (will be added in #1355).

@MarcCote MarcCote requested review from ranveeraggarwal and dmreagan Mar 4, 2018

@MarcCote MarcCote force-pushed the MarcCote:enh_ui_small_refactor branch from 7f3a879 to f305818 Mar 4, 2018

@MarcCote MarcCote force-pushed the MarcCote:enh_ui_small_refactor branch from f305818 to 341a306 Mar 4, 2018

@ranveeraggarwal

This is definitely much cleaner. We can probably add a bunch of beginner-friendly issues to make more use of numpy arrays like this for other elements 😄

if np.any(coords < 0) or np.any(coords > 1):
raise ValueError("Normalized coordinates must be in [0,1].")
coords = coords * self.size

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Mar 4, 2018

Member

Are we sure we want to do this instead of an extra parameter?

Here's another idea. If the parameter is specified as relative, we can normalize the values ourselves and go for a relative placement in this fashion.

This comment has been minimized.

@MarcCote

MarcCote Mar 4, 2018

Contributor

As an end user, you would have to know what pixel coordinate corresponds to let say 0.5 of the panel. I found it a bit annoying compared to just use 0.5. Also, if we ever made the panel resizable, specifying 0.5 is more intuitive I find.

Are there any cons I don't see?

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Mar 4, 2018

Member

Yeah, I guess let's go with this for now. This is intuitive enough.

coords = coords * self.size
#TODO: Check if coords is outside the panel.

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Mar 4, 2018

Member

I think coordinates going outside the panel is fine (in the absolute case). We should just throw a warning if this happens.

This comment has been minimized.

@MarcCote

MarcCote Mar 4, 2018

Contributor

I'm not a fan of raising a warning but I agree with you we should allow coordinates going outside the panel as there might be cases that could be useful. I'll just remove that comment, then.

@MarcCote MarcCote referenced this pull request Mar 4, 2018

Merged

WIP: ENH: UI Listbox #1355

@codecov-io

This comment has been minimized.

codecov-io commented Mar 4, 2018

Codecov Report

Merging #1441 into master will increase coverage by 0.01%.
The diff coverage is 97.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1441      +/-   ##
==========================================
+ Coverage   87.41%   87.42%   +0.01%     
==========================================
  Files         239      239              
  Lines       30568    30378     -190     
  Branches     3289     3263      -26     
==========================================
- Hits        26720    26558     -162     
+ Misses       3078     3058      -20     
+ Partials      770      762       -8
Impacted Files Coverage Δ
dipy/viz/tests/test_ui.py 85.98% <100%> (+0.56%) ⬆️
dipy/viz/ui.py 89.8% <96.96%> (+0.51%) ⬆️
dipy/data/fetcher.py 45.34% <0%> (-1.17%) ⬇️
dipy/segment/clustering.py 93.66% <0%> (+0.14%) ⬆️
dipy/segment/tests/test_qbx.py 96.92% <0%> (+0.14%) ⬆️
dipy/reconst/forecast.py 92.22% <0%> (+2.07%) ⬆️

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 f9fa0bd...2ec9d2b. Read the comment docs.

@dmreagan dmreagan added this to UI PR needs a review in Viz Module Mar 5, 2018

panel.add_element(button_example, 'relative', (0.2, 0.2))
panel.add_element(second_button_example, 'absolute', (480, 100))
panel.add_element(button_example, (0.2, 0.2))
panel.add_element(second_button_example, (480, 100))

This comment has been minimized.

@skoudoro

skoudoro Mar 6, 2018

Member

Now, this button seems to be outside the panel, I think you should update the position if it is not on purpose.

This comment has been minimized.

@MarcCote

MarcCote Mar 6, 2018

Contributor

Done

@skoudoro

This comment has been minimized.

Member

skoudoro commented Mar 6, 2018

I think it is ready to go. is it ok for you @ranveeraggarwal? Can you have a look @Garyfallidis?

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Mar 7, 2018

Wait! I think it would be better to merge #1448 instead of this one.

@skoudoro

This comment has been minimized.

Member

skoudoro commented Mar 7, 2018

so can you close this one?

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Mar 8, 2018

Sure.

@MarcCote MarcCote closed this Mar 8, 2018

@dmreagan dmreagan removed this from UI PR needs a review in Viz Module Mar 8, 2018

@MarcCote MarcCote deleted the MarcCote:enh_ui_small_refactor branch Jun 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment