Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Bugfix slicer #189

Merged
merged 3 commits into from
Jan 10, 2020
Merged

Bugfix slicer #189

merged 3 commits into from
Jan 10, 2020

Conversation

prisae
Copy link
Member

@prisae prisae commented Oct 7, 2019

Bugfix. Slicer failed if xslice was provided without a yslice, because of wrong comparison.

@prisae prisae requested a review from jcapriot October 14, 2019 11:03
@prisae
Copy link
Member Author

prisae commented Oct 29, 2019

Thought I'd ping this one, as it is really a tiny change which is required and nothing happened for three weeks...

@prisae
Fix wrong colour for fullspaces
c2a8300 
Another bug-fix: When the model was a homogenous fullspace, the xy- and
xz-plots had a wrong colour. yz was initially fine, but as soon as
scrolled would also have a wrong colour. The issue is with colorbar()
and only one value. So I introduced a +/- 1% range for the cases of
homogeneous fullspaces.
@codecov
Copy link

codecov bot commented Jan 4, 2020 •
edited
Loading

Codecov Report

Merging #189 into master will decrease coverage by 0.11%.
The diff coverage is 72.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #189      +/-   ##
==========================================
- Coverage   82.14%   82.02%   -0.12%     
==========================================
  Files          23       23              
  Lines        5006     5051      +45     
==========================================
+ Hits         4112     4143      +31     
- Misses        894      908      +14
Impacted Files Coverage Δ
discretize/Tests.py 83.17% <100%> (+4.2%) ⬆️
discretize/View.py 72.76% <71.15%> (-0.36%) ⬇️
discretize/utils/codeutils.py 82.85% <0%> (-2.86%) ⬇️
discretize/utils/matutils.py 92.9% <0%> (-2.03%) ⬇️
discretize/utils/meshutils.py 93.38% <0%> (-0.37%) ⬇️

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 c2a8300...aec9290. Read the comment docs.

@prisae
Copy link
Member Author

prisae commented Jan 4, 2020

I added another bugfix to the slicer. I'd be really happy if we could move these in at some point.

@prisae
Copy link
Member Author

prisae commented Jan 4, 2020

Here graphically what this last bugfix fixes:

Before, if you provided a single value, it looked like this:
Selection_002
So the colour in the xy- and xz-plots is wrong. As soon as you scroll, the colour in the yz-plot will change to the wrong colour too.

With the bug-fix, they all get the right colour, and will remain correct if scrolling:
Selection_001

The bug only affects homogeneous fullspaces, which caused an issue with the range used by colorbar().

jcapriot
jcapriot approved these changes Jan 10, 2020
View reviewed changes
Copy link
Member

@jcapriot jcapriot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for the small bug fixes @prisae.

@jcapriot jcapriot merged commit bcde4c5 into master Jan 10, 2020
@jcapriot jcapriot deleted the fix_slicer branch January 10, 2020 20:42
@prisae
Copy link
Member Author

prisae commented Jan 10, 2020

Thanks for reviewing/merging. I am the one to blame introducing them in the first place...

@jcapriot
Copy link
Member

Also means someone approved them in the first place... no worries

@prisae prisae mentioned this pull request Sep 24, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcapriot jcapriot jcapriot approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@prisae @jcapriot