corrections to the pr to the pr to the pr #10
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bmorris3
commented
May 19, 2023
| @@ -169,7 +173,8 @@ def _parse_image(app, file_obj, data_label, ext=None): | |||
| # for outside_*_bounding_box should also be updated. | |||
| data.coords._orig_bounding_box = data.coords.bounding_box | |||
| data.coords.bounding_box = None | |||
| data_label = app.return_data_label(data_label, alt_name="image_data") | |||
| if not data.meta.get(wcs_only_key, False): | |||
| data_label = app.return_data_label(data_label, alt_name="image_data") | |||
There was a problem hiding this comment.
Without this correction, the data label still gets appended with "DATA".
bmorris3
commented
May 19, 2023
| arr = ndd.data | ||
| data = Data(label=data_label) | ||
| data.meta.update(standardize_metadata(ndd.meta)) | ||
| data.coords = ndd.wcs | ||
| component = Component.autotyped(arr, units="") | ||
| data.add_component(component=component, label="DATA") | ||
| return data | ||
| yield (data, data_label) |
There was a problem hiding this comment.
I didn't think it would be inside the parser, but now that it is, yes, it has to be a generator.
Also maybe rename to _wcsonly_to_glue_data to be consistent with other generator functions.
bmorris3
commented
May 19, 2023
Comment on lines
142
to
145
| if file_obj.meta.get(wcs_only_key, False): | ||
| data_iter = _wcsonly2data(file_obj, data_label) | ||
| else: | ||
| data_iter = _nddata_to_glue_data(file_obj, data_label) |
There was a problem hiding this comment.
Your PR never actually used _wcsonly2data, so I invoke it here.
pllim
reviewed
May 19, 2023
| @@ -26,6 +26,7 @@ | |||
|
|
|||
| INFO_MSG = ("The file contains more viewable extensions. Add the '[*]' suffix" | |||
| " to the file name to load all of them.") | |||
| wcs_only_key = "_WCS_ONLY" | |||
There was a problem hiding this comment.
Can we use the app attribute instead of re-defining it here?
pllim
reviewed
May 19, 2023
| @@ -169,7 +173,8 @@ def _parse_image(app, file_obj, data_label, ext=None): | |||
| # for outside_*_bounding_box should also be updated. | |||
| data.coords._orig_bounding_box = data.coords.bounding_box | |||
| data.coords.bounding_box = None | |||
| data_label = app.return_data_label(data_label, alt_name="image_data") | |||
| if not data.meta.get(wcs_only_key, False): | |||
There was a problem hiding this comment.
Should we use the layer_is_wcs_only check here?
There was a problem hiding this comment.
We could but it's the same check?
pllim
reviewed
May 19, 2023
| @@ -407,11 +412,11 @@ def _ndarray_to_glue_data(arr, data_label): | |||
| # ---- Functions that handle WCS-only data ----- | |||
|
|
|||
| def _wcsonly2data(ndd, data_label): | |||
| """Return Data given NDData containly WCS-only data.""" | |||
| """Return Data given NDData containing WCS-only data.""" | |||
pllim
reviewed
May 19, 2023
| @@ -383,7 +383,7 @@ def _prepare_rotated_nddata(real_image_shape, wcs, rotation_angle, refdata_shape | |||
| # create a fake NDData (we use arange so data boundaries show up in Imviz | |||
| # if it ever is accidentally exposed) with the rotated GWCS: | |||
| ndd = NDData( | |||
| data=np.arange(sum(refdata_shape), dtype=np.int8).reshape(refdata_shape), | |||
| data=np.arange(np.prod(refdata_shape), dtype=np.int8).reshape(refdata_shape), | |||
There was a problem hiding this comment.
Hmm does plain sum not work for you? The shape should be a plain tuple if ints.
There was a problem hiding this comment.
If shape is (10, 10), sum is 20. We want the product (100).
There was a problem hiding this comment.
Oh, right... I didn't catch that with (2, 2). Doh!
pllim
reviewed
May 19, 2023
| @@ -383,7 +383,7 @@ def _prepare_rotated_nddata(real_image_shape, wcs, rotation_angle, refdata_shape | |||
| # create a fake NDData (we use arange so data boundaries show up in Imviz | |||
| # if it ever is accidentally exposed) with the rotated GWCS: | |||
| ndd = NDData( | |||
| data=np.arange(sum(refdata_shape), dtype=np.int8).reshape(refdata_shape), | |||
| data=np.arange(np.prod(refdata_shape), dtype=np.int8).reshape(refdata_shape), | |||
There was a problem hiding this comment.
Oh, right... I didn't catch that with (2, 2). Doh!
Co-authored-by: P. L. Lim <[email protected]>
Co-authored-by: P. L. Lim <[email protected]>
|
Thanks! I'll merge this and then clean up my PR to your PR. Hang in there! |
pllim
added a commit
that referenced
this pull request
Nov 3, 2023
#4) * Lim edits * Improve verbiage in docstring Co-authored-by: Brett M. Morris <[email protected]> * Address review comments from bmorris3 * Add multi WCS-only test back * Update concept notebook * corrections to the pr to the pr to the pr (#10) * corrections to the pr to the pr to the pr * using app-level attr * Update jdaviz/configs/imviz/plugins/parsers.py Co-authored-by: P. L. Lim <[email protected]> * Update jdaviz/configs/imviz/plugins/parsers.py Co-authored-by: P. L. Lim <[email protected]> --------- Co-authored-by: P. L. Lim <[email protected]> * Follow up to PR to my PR to your PR * WCS-only must be loaded into viewer but not visible. Fix concept notebook --------- Co-authored-by: Brett M. Morris <[email protected]>
pllim
added a commit
that referenced
this pull request
Nov 15, 2023
#4) * Lim edits * Improve verbiage in docstring Co-authored-by: Brett M. Morris <[email protected]> * Address review comments from bmorris3 * Add multi WCS-only test back * Update concept notebook * corrections to the pr to the pr to the pr (#10) * corrections to the pr to the pr to the pr * using app-level attr * Update jdaviz/configs/imviz/plugins/parsers.py Co-authored-by: P. L. Lim <[email protected]> * Update jdaviz/configs/imviz/plugins/parsers.py Co-authored-by: P. L. Lim <[email protected]> --------- Co-authored-by: P. L. Lim <[email protected]> * Follow up to PR to my PR to your PR * WCS-only must be loaded into viewer but not visible. Fix concept notebook --------- Co-authored-by: Brett M. Morris <[email protected]>
pllim
added a commit
that referenced
this pull request
Dec 8, 2023
#4) * Lim edits * Improve verbiage in docstring Co-authored-by: Brett M. Morris <[email protected]> * Address review comments from bmorris3 * Add multi WCS-only test back * Update concept notebook * corrections to the pr to the pr to the pr (#10) * corrections to the pr to the pr to the pr * using app-level attr * Update jdaviz/configs/imviz/plugins/parsers.py Co-authored-by: P. L. Lim <[email protected]> * Update jdaviz/configs/imviz/plugins/parsers.py Co-authored-by: P. L. Lim <[email protected]> --------- Co-authored-by: P. L. Lim <[email protected]> * Follow up to PR to my PR to your PR * WCS-only must be loaded into viewer but not visible. Fix concept notebook --------- Co-authored-by: Brett M. Morris <[email protected]>
pllim
added a commit
that referenced
this pull request
Dec 8, 2023
#4) * Lim edits * Improve verbiage in docstring Co-authored-by: Brett M. Morris <[email protected]> * Address review comments from bmorris3 * Add multi WCS-only test back * Update concept notebook * corrections to the pr to the pr to the pr (#10) * corrections to the pr to the pr to the pr * using app-level attr * Update jdaviz/configs/imviz/plugins/parsers.py Co-authored-by: P. L. Lim <[email protected]> * Update jdaviz/configs/imviz/plugins/parsers.py Co-authored-by: P. L. Lim <[email protected]> --------- Co-authored-by: P. L. Lim <[email protected]> * Follow up to PR to my PR to your PR * WCS-only must be loaded into viewer but not visible. Fix concept notebook --------- Co-authored-by: Brett M. Morris <[email protected]>
pllim
added a commit
that referenced
this pull request
Jan 23, 2024
#4) * Lim edits * Improve verbiage in docstring Co-authored-by: Brett M. Morris <[email protected]> * Address review comments from bmorris3 * Add multi WCS-only test back * Update concept notebook * corrections to the pr to the pr to the pr (#10) * corrections to the pr to the pr to the pr * using app-level attr * Update jdaviz/configs/imviz/plugins/parsers.py Co-authored-by: P. L. Lim <[email protected]> * Update jdaviz/configs/imviz/plugins/parsers.py Co-authored-by: P. L. Lim <[email protected]> --------- Co-authored-by: P. L. Lim <[email protected]> * Follow up to PR to my PR to your PR * WCS-only must be loaded into viewer but not visible. Fix concept notebook --------- Co-authored-by: Brett M. Morris <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR inception continues. Corrections for bmorris3#4 (for spacetelescope#2179).