layer-lettering to handle 2 "digits" (up to 700 layers) #1850

kecnry · 2022-11-16T15:14:00Z

Description

(Ok, technically 702 layers). This pull request improves support for more than 26 layers by lettering with "AA" through "ZZ" instead of going to random characters that happen to be after char(97 + 26).

For inputs larger than 701, this will begin to use non-alphabet characters in the first digit. 702 gives '{a', for example.

Change log entry

Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
Do the proposed changes follow the STScI Style Guides?
Are tests added/updated as required? If so, do they follow the STScI Style Guides?
Are docs added/updated as required? If so, do they follow the STScI Style Guides?
Did the CI pass? If not, are the failures related?
Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

codecov · 2022-11-16T15:35:33Z

Codecov Report

Base: 88.29% // Head: 88.30% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (00b530b) compared to base (b5bbccd).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1850   +/-   ##
=======================================
  Coverage   88.29%   88.30%           
=======================================
  Files          95       95           
  Lines       10396    10404    +8     
=======================================
+ Hits         9179     9187    +8     
  Misses       1217     1217

Impacted Files	Coverage Δ
jdaviz/app.py	`94.15% <100.00%> (ø)`
jdaviz/utils.py	`90.08% <100.00%> (+0.70%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

kecnry · 2022-11-16T16:55:34Z

do we need test coverage for this? I could move the function out of the method and just test it directly, but I also don't expect that we'd touch this code often/ever, so maybe we can ignore it 🤷 I definitely don't think we want a test that loads dozens of entries to force the code to go through this logic though, right?

pllim

Why not just cycle through unicode to increase max limit to 100k or so? 😉

jdaviz/app.py

jdaviz/tests/test_utils.py

pllim · 2022-11-16T21:51:36Z

jdaviz/utils.py

@@ -175,6 +175,21 @@ def bqplot_clear_figure(fig):
    setattr(fig, 'axis_registry', {})


+def alpha_index(index):
+    """Converts an index to A-Z, AA-ZZ


Since this might be used by other devs down the road, would be nice to expand a little bit with proper numpydoc and maybe even a short example, so they don't have to dig the test code to find out.

Also do we have to worry about someone providing negative index? Is that possible?

this is why I originally had it nested in the method as its not really intended to be used generally 😜 Negative index would give non-alphabet characters as long as 97-index >= 0 and would otherwise raise an error. I can catch and raise errors for negative indices if we want to keep this general, but that would never happen in its current use since it is always passed a length as the index.

Okay, just want to make sure we'll never do fancy slicing numpy-style.

jdaviz/utils.py

* full api doc description * better range checking and errors * use pytest.mark.parameterize * accept input larger than 701 (although starts to return non-alphabet strings)

pllim · 2022-11-17T16:33:35Z

jdaviz/utils.py

+    """
+    # if we ever want to support more than 702 layers, then we'll need a third
+    # "digit" and will need to account for the horizontal space in the legends
+    if not isinstance(index, int):  # pragma: no cover


Would still be nice to test the exceptions but as a separate test function from the parametrized one. Sorry for the extra work!

The check for index > 701 is gone. Is there no more max limit?

no, I decided to remove it. I don't think layer-lettering is a good place to enforce a maximum number of layers with a confusing traceback. As noted in the comments and the description of the PR, this will start to introduce special characters after index 701 (which was the previous behavior after 25). If that becomes a common use-case, then we can extend this logic to three (or N) digits and will need to update the styling of all components to handle the extra horizontal space required.

CHANGES.rst

jdaviz/utils.py

rosteen

Looks good to me, I loaded a bunch of spectra and confirmed that it successfully goes into the double digits.

jdaviz/tests/test_utils.py

jdaviz/utils.py

Co-authored-by: P. L. Lim <[email protected]>

pllim

Feel free to merge when CI passes. Thanks!

… (up to 700 layers)

…0-on-v3.1.x Backport PR #1850 on branch v3.1.x (layer-lettering to handle 2 "digits" (up to 700 layers))

kecnry added this to the 3.1.1 milestone Nov 16, 2022

kecnry force-pushed the layer-letter-lengthening branch 2 times, most recently from 081c493 to 6253904 Compare November 16, 2022 15:16

layer-lettering to handle 2 digits (up to 700 layers)

b7a59f0

kecnry force-pushed the layer-letter-lengthening branch from 6253904 to b7a59f0 Compare November 16, 2022 15:23

kecnry marked this pull request as ready for review November 16, 2022 15:30

kecnry requested review from duytnguyendtn, rosteen, javerbukh, ojustino, pllim and bmorris3 as code owners November 16, 2022 15:30

pllim added bug Something isn't working 💤backport-v3.1.x on-merge: backport to v3.1.x labels Nov 16, 2022

pllim reviewed Nov 16, 2022

View reviewed changes

jdaviz/app.py Outdated Show resolved Hide resolved

move function to utils and add test coverage

8327d93

kecnry requested a review from pllim November 16, 2022 20:05

pllim reviewed Nov 16, 2022

View reviewed changes

jdaviz/tests/test_utils.py Outdated Show resolved Hide resolved

pllim reviewed Nov 16, 2022

View reviewed changes

jdaviz/utils.py Outdated Show resolved Hide resolved

respond to code review

c02c8fa

* full api doc description * better range checking and errors * use pytest.mark.parameterize * accept input larger than 701 (although starts to return non-alphabet strings)

kecnry requested a review from pllim November 17, 2022 14:10

pllim reviewed Nov 17, 2022

View reviewed changes

CHANGES.rst Outdated Show resolved Hide resolved

pllim reviewed Nov 17, 2022

View reviewed changes

jdaviz/utils.py Outdated Show resolved Hide resolved

jdaviz/utils.py Outdated Show resolved Hide resolved

jdaviz/utils.py Outdated Show resolved Hide resolved

jdaviz/utils.py Outdated Show resolved Hide resolved

kecnry force-pushed the layer-letter-lengthening branch from 2a13ce8 to 9b24e60 Compare November 18, 2022 16:33

rosteen approved these changes Nov 18, 2022

View reviewed changes

kecnry added the Ready for final review label Nov 18, 2022

pllim reviewed Nov 18, 2022

View reviewed changes

jdaviz/tests/test_utils.py Show resolved Hide resolved

jdaviz/utils.py Outdated Show resolved Hide resolved

jdaviz/utils.py Outdated Show resolved Hide resolved

Apply suggestions from code review

00b530b

Co-authored-by: P. L. Lim <[email protected]>

kecnry force-pushed the layer-letter-lengthening branch from 2ef2eee to 00b530b Compare November 18, 2022 18:17

pllim approved these changes Nov 18, 2022

View reviewed changes

kecnry merged commit 4e31443 into spacetelescope:main Nov 18, 2022

kecnry deleted the layer-letter-lengthening branch November 18, 2022 19:25

meeseeksmachine mentioned this pull request Nov 18, 2022

Backport PR #1850 on branch v3.1.x (layer-lettering to handle 2 "digits" (up to 700 layers)) #1857

Merged

meeseeksmachine pushed a commit to meeseeksmachine/jdaviz that referenced this pull request Nov 18, 2022

Backport PR spacetelescope#1850: layer-lettering to handle 2 "digits"…

bda0f96

… (up to 700 layers)

pllim added a commit that referenced this pull request Nov 18, 2022

Merge pull request #1857 from meeseeksmachine/auto-backport-of-pr-185…

e88a294

…0-on-v3.1.x Backport PR #1850 on branch v3.1.x (layer-lettering to handle 2 "digits" (up to 700 layers))

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

layer-lettering to handle 2 "digits" (up to 700 layers) #1850

layer-lettering to handle 2 "digits" (up to 700 layers) #1850

kecnry commented Nov 16, 2022 •

edited

Loading

codecov bot commented Nov 16, 2022 •

edited

Loading

kecnry commented Nov 16, 2022

pllim left a comment

pllim Nov 16, 2022

pllim Nov 16, 2022 •

edited

Loading

kecnry Nov 16, 2022

pllim Nov 16, 2022

pllim Nov 17, 2022

pllim Nov 17, 2022

kecnry Nov 17, 2022

rosteen left a comment

pllim left a comment

layer-lettering to handle 2 "digits" (up to 700 layers) #1850

layer-lettering to handle 2 "digits" (up to 700 layers) #1850

Conversation

kecnry commented Nov 16, 2022 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

codecov bot commented Nov 16, 2022 • edited Loading

Codecov Report

kecnry commented Nov 16, 2022

pllim left a comment

Choose a reason for hiding this comment

pllim Nov 16, 2022

Choose a reason for hiding this comment

pllim Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

kecnry Nov 16, 2022

Choose a reason for hiding this comment

pllim Nov 16, 2022

Choose a reason for hiding this comment

pllim Nov 17, 2022

Choose a reason for hiding this comment

pllim Nov 17, 2022

Choose a reason for hiding this comment

kecnry Nov 17, 2022

Choose a reason for hiding this comment

rosteen left a comment

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

kecnry commented Nov 16, 2022 •

edited

Loading

codecov bot commented Nov 16, 2022 •

edited

Loading

pllim Nov 16, 2022 •

edited

Loading