Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Ellipse: Do not throw error for theta=0 #2280

Merged
merged 1 commit into from
Mar 17, 2022
Merged

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Mar 9, 2022

Description

theta=0 is the same as no rotation, so throwing NotImplementedError for that case is confusing.

@codecov
Copy link

codecov bot commented Mar 9, 2022

Codecov Report

Merging #2280 (519874b) into main (55428b0) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2280      +/-   ##
==========================================
- Coverage   88.09%   88.09%   -0.01%     
==========================================
  Files         247      247              
  Lines       23293    23293              
==========================================
- Hits        20521    20520       -1     
- Misses       2772     2773       +1     
Impacted Files Coverage Δ
glue/core/roi.py 91.92% <100.00%> (ø)
glue/conftest.py 66.25% <0.00%> (-1.25%) ⬇️

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 55428b0...519874b. Read the comment docs.

@dhomeier
Copy link
Collaborator

This would be covered by #2235, but that's a somewhat larger PR, so might be preferable to put this in as a temporary fix. Not quite sure why the existing version even has a theta parameter.

@pllim
Copy link
Contributor Author

pllim commented Mar 10, 2022

Up to you. This patch is not critical for my work. I noticed the error and it didn't make sense to me.

@astrofrog
Copy link
Member

I'll merge this as no real downside even though theta shouldn't be set in the first place.

@astrofrog astrofrog merged commit 1843787 into glue-viz:main Mar 17, 2022
@pllim pllim deleted the patch-1 branch March 17, 2022 11:18
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants