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

Imviz regions setter/getter API #721

Merged
merged 2 commits into from
Jul 14, 2021

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Jul 6, 2021

Fix #622. Second attempt of #630. (It is easier to open a new PR than to resolve conflicts of that one...)

Note to self: Open follow-up issues about modifying regions, roundtripping, GWCS support...

@github-actions github-actions bot added documentation Explanation of code and concepts imviz labels Jul 6, 2021
@pllim pllim mentioned this pull request Jul 6, 2021
7 tasks
@pllim pllim added this to the Imviz 1.0 milestone Jul 6, 2021
@codecov
Copy link

codecov bot commented Jul 6, 2021

Codecov Report

Merging #721 (650d13c) into main (e4b61d2) will increase coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #721      +/-   ##
==========================================
+ Coverage   62.40%   62.52%   +0.12%     
==========================================
  Files          65       65              
  Lines        4237     4267      +30     
==========================================
+ Hits         2644     2668      +24     
- Misses       1593     1599       +6     
Impacted Files Coverage Δ
jdaviz/app.py 79.30% <100.00%> (+0.10%) ⬆️
jdaviz/configs/imviz/helper.py 98.99% <100.00%> (+0.16%) ⬆️
...configs/default/plugins/data_tools/file_chooser.py 65.71% <0.00%> (-3.43%) ⬇️

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 e4b61d2...650d13c. Read the comment docs.

@pllim pllim marked this pull request as ready for review July 6, 2021 21:45
@pllim pllim requested review from astrofrog and eteq July 6, 2021 21:48
@pllim
Copy link
Contributor Author

pllim commented Jul 6, 2021

So, using "reference data" got around the problem I encountered in #630 . 😸

Copy link
Collaborator

@rosteen rosteen 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, I tested the code in the example notebook and it works. The API makes sense to me. I noted some performance issues in #723, I don't think they need to hold up this ticket but should be a priority to fix, in my opinion.

Copy link
Contributor

@ibusko ibusko left a comment

Choose a reason for hiding this comment

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

Code looks good. I tested with the example notebook and it works as expected.

@pllim pllim force-pushed the imviz-region-loader-api-2 branch from e358949 to 650d13c Compare July 14, 2021 19:22
@pllim
Copy link
Contributor Author

pllim commented Jul 14, 2021

Given there are 2 approvals, I am merging. Thanks, everyone!

p.s. I ran it locally after rebase and nothing broke. The lag is very apparent but out of scope, as Ricky said.

@pllim pllim merged commit 7603180 into spacetelescope:main Jul 14, 2021
@pllim pllim deleted the imviz-region-loader-api-2 branch July 14, 2021 19:55
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
documentation Explanation of code and concepts imviz Ready for final review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JDAT-1472: Imviz regions setters/getters in the helper
3 participants