Update the interface and ginga implementation to make some changes of the marker API #173
Conversation
mwcraig
added
enhancement
| rng = np.random.default_rng(1234) | ||
| data = rng.integers(0, 100, (5, 2)) |
There was a problem hiding this comment.
Is randomness really necessary here?
There was a problem hiding this comment.
No, that is just what GitHub Copilot inserted 😬
There was a problem hiding this comment.
As discussed today, I will review this ignoring potential refactoring we need for regions support.
Not a fan of the recursion in Ginga implementation but that works for now. We might have to refactor again anyway.
I don't think you need np.random. The test data is small enough and the context is irrelevant, so something like np.arange(10).reshape((5, 2)) would work just as well.
| self.image.add_markers(tab, marker_name='test2') | ||
|
|
||
| self.image.remove_markers(marker_name='all') | ||
| assert self.image.get_markers(marker_name='all') is None |
There was a problem hiding this comment.
If we return empty table instead, this test will need to be updated, no?
| self.image.add_markers(tab, marker_name='test2') | ||
|
|
||
| self.image.remove_markers(marker_name=['test1', 'test2']) | ||
| assert self.image.get_markers(marker_name='all') is None |
There was a problem hiding this comment.
If we return empty table instead, this test will need to be updated, no?
There was a problem hiding this comment.
Is the empty table thingy part of this PR or separate PR?
There was a problem hiding this comment.
Since we are meeting later today it may as well be part of this PR. If the next meeting was pretty far off I'd be inclined to keep it spearate.
|
Thanks, @pllim -- I'll hold off on changing the return values until after today's meeting. |
| with warnings.catch_warnings(): | ||
| warnings.simplefilter("error") |
There was a problem hiding this comment.
Hmm, won't setting pytest to turn all warnings into exceptions take care of this?
|
|
||
| def test_remove_markers(self): | ||
| with pytest.raises(ValueError, match='arf'): | ||
| self.image.remove_markers(marker_name='arf') | ||
|
|
||
| def test_remove_markers_name_all(self): | ||
| rng = np.random.default_rng(1234) |
There was a problem hiding this comment.
There are remaining unnecessary random number generators that should be replaced with static arrays.
| self._check_marker_table_return_properties(self.image.get_markers(marker_name='all')) | ||
|
|
||
| def test_remove_marker_accepts_list(self): | ||
| rng = np.random.default_rng(1234) |
|
Lights are green, so this is ready for another look |
There was a problem hiding this comment.
In the light of spacetelescope/jdaviz#2410 , I find that the API will be significantly different if we were to go with Regions instead of Table.
So, do you want to merge this anyway and refactor again later, or wait for that discussion first?
|
Right, forgot about that. Let's wait until after the next meeting. |
Co-authored-by: P. L. Lim <[email protected]>
This behavior is tested in the widget API tests
Once upon a time, get_markers raised a ValueError if the marker_name passed in doesn't match any existing marker name. Later, the API was changed so that a warning was issued and None was returned if the marker name was not found. In the most recent iteration of the API, an empty table is to be returned with the proper column names. It turns out three responses were being checked. In one case, the test checked (and passed) if a warning was raised. In another, the test passed if a ValueError was raised. The setup for those cases remains but now the check is that the return table has now rows and has the correct column names.
Co-authored-by: P. L. Lim <[email protected]>
Addresses review comments
mwcraig
force-pushed
the
update-marker-access
branch
from
5783b46 to
a240d9f
Compare
No description provided.